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

Install RuboCop Rails to actually run RuboCop in CI #4421

Open
wants to merge 2 commits into
base: development
Choose a base branch
from

Conversation

bencomp
Copy link
Contributor

@bencomp bencomp commented Nov 21, 2024

I noticed that the "Run RuboCop" step in CI is always skipped, because RuboCop is not in the Gemfile(.lock). This PR adds RuboCop and RuboCop Rails. I followed https://github.com/rubocop/rubocop-rails#rubocop-configuration-file to add the .rubocop.yml file, although there is more to configure, as the output of RuboCop shows in https://github.com/bencomp/fromthepage/actions/runs/11937781469/job/33274542417.

@benwbrum benwbrum requested a review from WillNigel23 November 21, 2024 14:39
Copy link
Collaborator

@WillNigel23 WillNigel23 left a comment

Choose a reason for hiding this comment

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

We are in the process of upgrading rails at the moment. That includes getting rubocop as part of the CI process.

The updates done here will only do warning for syntax. We plan to get to this sometime after the rails upgrade (as there are syntax changes too that are different from older versions I believe). For now I think we should leave it as is.

@bencomp
Copy link
Contributor Author

bencomp commented Nov 26, 2024

Thanks for reviewing! However, I'm not sure I understand your comments.

RuboCop is already defined as a step in the CI process, i.e. the GitHub workflow. Currently it always fails because it isn't installed in the CI environment. This step is allowed to fail, so every time it runs, it produces an ignored error.
By default RuboCop runs as a linter that flags problems, without trying to fix them. In CI that is a good thing. Since there are very many violations of code style currently, it is probably a good thing to allow the RuboCop step to keep failing.
In this sense I'm leaving it as is.

I understand that Ruby 3 syntax is slightly different, but I don't think that matters here. As far as I can tell, RuboCop is smart and understands the version that you are using. I don't think this PR gets in the way of upgrading Rails.

Perhaps it is friendlier to contributors to eventually only run RuboCop on the code that they change/add in a PR, after deciding what (style) rules you want people to adhere to. Perhaps there are smarter ways of running RuboCop too, like running it with Reviewdog in a separate job.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants