Skip to content
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

Merged
merged 5 commits into from May 16, 2024
Merged

Rubocop check #15416

merged 5 commits into from May 16, 2024

Conversation

toy
Copy link
Contributor

@toy toy commented Apr 29, 2024

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)

@toy toy force-pushed the rubocop-check branch 2 times, most recently from 1646bc5 to b30650f Compare April 30, 2024 11:54
@toy toy force-pushed the rubocop-check branch 2 times, most recently from 3512407 to debcff1 Compare May 10, 2024 10:26
@toy toy marked this pull request as ready for review May 13, 2024 14:23
@cbliard
Copy link
Member

cbliard commented May 13, 2024

Why is a todo file needed if it runs only on modified files?

@toy
Copy link
Contributor Author

toy commented May 13, 2024

@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.

@cbliard
Copy link
Member

cbliard commented May 14, 2024

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:

FactoryBot/AssociationStyle:
  Exclude:
    - 'modules/backlogs/spec/factories/impediment_factory.rb'
    - 'modules/backlogs/spec/factories/task_factory.rb'
    - 'modules/budgets/spec/factories/budget_factory.rb'
    - ...

I ran rubocop --only FactoryBot/AssociationStyle modules/backlogs/spec/factories/impediment_factory.rb just to see what the problem is, but it emits no warning. I thought it was a pending cop so I added --enable-pending-cops but same result. And then while writing this message I realized I have to remove the line from the todo first 🙃 and then I could see the issues.

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.

@toy
Copy link
Contributor Author

toy commented May 14, 2024

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

@toy
Copy link
Contributor Author

toy commented May 14, 2024

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

@cbliard
Copy link
Member

cbliard commented May 14, 2024

Great! I'm all for removing the todo files, and then adding them later if we feel it's needed.

Copy link
Member

@cbliard cbliard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds great!

@toy toy merged commit d5d0311 into dev May 16, 2024
9 checks passed
@toy toy deleted the rubocop-check branch May 16, 2024 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants