-
-
Notifications
You must be signed in to change notification settings - Fork 478
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
Open
thejonroberts
wants to merge
22
commits into
rubyforgood:main
Choose a base branch
from
thejonroberts:standard-to-rubocop-plus-plugins
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Standard.rb linting -> Rubocop and relevant plugins #6086
thejonroberts
wants to merge
22
commits into
rubyforgood:main
from
thejonroberts:standard-to-rubocop-plus-plugins
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
thejonroberts
requested review from
compwron,
FireLemons and
elasticspoon
as code owners
October 23, 2024 18:47
github-actions
bot
added
dependencies
Pull requests that update a dependency file
ruby
Pull requests that update Ruby code
Tests! 🎉💖👏
labels
Oct 23, 2024
Offense count by file
|
thejonroberts
changed the title
Standard.rb linting -> Rubocop and relavent plugins
Standard.rb linting -> Rubocop and relevant plugins
Oct 23, 2024
thejonroberts
force-pushed
the
standard-to-rubocop-plus-plugins
branch
from
November 4, 2024 17:09
e506cb8
to
e46cb3c
Compare
considered deprecated, config to be removed by rspec-rails
Seems like a good reason... a big change tho. |
Probably yes but I or someone will get back to you |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
dependencies
Pull requests that update a dependency file
ruby
Pull requests that update Ruby code
Tests! 🎉💖👏
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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