-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rubocop check #15416
Rubocop check #15416
Conversation
1646bc5
to
b30650f
Compare
3512407
to
debcff1
Compare
Why is a todo file needed if it runs only on modified files? |
@cbliard Mostly to not force resolving bigger issues (like complexity) even when doing small changes right away. I agree that as reviewdog will even reduce comments only to changed lines, it may be not really needed. |
What I do not like when working with todo like this is that the autocorrect "randomly" does not work as some files are excluded from some rules. I do not really see how it can help us write better code if everything is hidden. Besides, I do not understand why some are disabled. It was generated automatically but still. For instance this part:
I ran I wonder how we will manage the todo file in the future. Meanwhile, I think it's very important to have rubocop enabled again and I am really grateful that you invested some time in it. I just have some fears about that todo file. I wonder what other people think about it. |
I agree, just worried that if reintroducing rubocop will create too much additional work, there will be a desire to get rid of it. Maybe a good option is to remove the todo list and add it separately if the checker will become problematic |
I checked how bad will it look if I for example autocorrect a bunch of quotes and it doesn't look to bad to fix the found violations, so unless there will be another opinion I'm going to remove todos |
Great! I'm all for removing the todo files, and then adding them later if we feel it's needed. |
…o changes in files with .rb extension
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds great!
Enable rubocop check using reviewdog action that will comment on all PRs. It was silently failing, as not all dependencies were enabled.
To not overwhelm with failures a todo file for all failures is included plus a separate file for disable directives (it is not generated automatically). The goal is to start checking all new changes and gradually empty the todo files.#15557 as an example of how many violations could there be, so removed todos (#15561 with the branch containing them)