-
-
Notifications
You must be signed in to change notification settings - Fork 487
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
Standard.rb linting -> Rubocop and relevant plugins #6086
Standard.rb linting -> Rubocop and relevant plugins #6086
Conversation
Offense count by file
|
e506cb8
to
e46cb3c
Compare
Seems like a good reason... a big change tho. |
Probably yes but I or someone will get back to you |
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.
We use standardrb so we don't need to configure it. This seems worse.
I do agree that default_scope is bad because it is a footgun |
@compwron Zero config is nice, but there are a few tradeoffs, imo... I'll make a separate PR using standard with the plugins rolled up into it instead, and we can compare the approaches. Defaults will be used for all the rubocop plugins. There will be more violations, but it can also generate a todo file. (I may try rubocop-rails rather than standard-rails, and see if it leaves the default scope rule enabled...) |
considered deprecated, config to be removed by rspec-rails
0e9e1b7
to
5e0172d
Compare
Dropping in favor of #6120 as standard can inherit some rubocop config, and therefore avoids changes to CI setup etc... |
What github issue is this PR for, if any?
No Issue.
What changed, and why?
This project welcomes and encourages new contributors who may not have a lot of experience with idiomatic ruby or common patterns to avoid in rails/rspec/capybara, etc... So having good linting in place is important -- both to maintain code quality and to give newer devs a tool to learn about common patterns & good practices.
It also makes review easier because there are fewer style nits & common gotchas to get hung up on. Maintainers can focus on more important things! And, to be honest, I love linters because I don't want to have to think about all these things!
It will also create a nice set of TODOs for people that want to start contributing! An Epic issue similar the the xit/skipped specs epic could be created to fix the todos file by file.
This rubocop setup is based on this evil martians article on adding standard/rubocop to a legacy project.
.rubocop.yml
bin/lint
, GitHub CI lint workflow, & git hooks.rubocop:disable
comments as any offenses will be in the todo file.I made a PR into this branch with all of the autocorrections here. If we want to go ahead and include those, we can just merge that into this PR. May cause some minor merge conflicts for works in progress. Even if not, the PR will give an idea of the impact this will have on the codebase.
Things to consider:
default_scope
is both tempting to use AND causes some tricky issues for the uninitiated. It is difficult to figure out when an.unscope
is needed because of a default scope. And. unscope
itself can cause unexpected behavior when used on associations. I have personally run into confusion around it many, many times!let!
pattern has been common in every project I personally have worked on... Therefore I left it enabled but with a detailed description. I think disabling it would be okay, but encouraging good practice won out in my opinion. I added a lengthy details example to better explain fix to inexperienced devs.create
for associations. I think this is a great rule! Using association over create facilitates use ofbuild_stubbed
, which chokes if any associations use create. Also, factory create cascades can lead to so much extra database time in specs, and become harder to fix the more complicated things get. So I left it enabled, and current violations are in the todo.--ignore-disable-comments
flag to the workflows to be super strict, but I think it's probably better to allow those rather than be tempted to tweak configs over and over. If a rule is slowing down a feature too much, justrubocop:todo
it and let someone else fix it later.How is this tested? (please write tests!) 💖💪
Current specs