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

Add rubocop binstub, simplify configuration #30407

Merged
merged 8 commits into from
Jun 13, 2024

Conversation

mjankowski
Copy link
Contributor

@mjankowski mjankowski commented May 23, 2024

Series of related changes:

  • Add bin/rubocop binstub for rubocop and configure CI linter call to use it
  • Re-organize the top-level .rubocop.yml file to ONLY declare the AllCops config and allow other files to come in via inherit_from config
  • Move all the previous per-department configuration into one file per department config in a .rubocop dir, doing some yaml sort/cleanup while moving
  • Once over on all the comments ... most of these seemed sort of pointless to me, basically re-stating what the rule does. I preserved the ones where in my judgement it was not totally obvious why the config was there.
  • Move the config for the linters which are defined by the repo (ie, not from external gems) to a custom.yml file (this is just the middledot thing now)
  • Add a "strict" config which gets loaded AFTER the todo, ensuring its cops always run on all files

No rules or config are actually changed here, but I do want to continue to work through the _todo file and some of the top-level Exclude I've moved here.

Many of these ideas are inspired by this approach: https://evilmartians.com/chronicles/rubocoping-with-legacy-bring-your-ruby-code-up-to-standard

I have a WIP branch which does a version of the "start with standardrb ruleset and then override" described in that blog post, but wanted to start with just the config portion.

@mjankowski
Copy link
Contributor Author

Added a few more ideas/commits here, updated description to reflect.

Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

Copy link
Contributor

This pull request has resolved merge conflicts and is ready for review.

Copy link
Contributor

@ClearlyClaire ClearlyClaire left a comment

Choose a reason for hiding this comment

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

What is the benefit of adding a binstup for rubocop?

.rubocop/strict.yml Outdated Show resolved Hide resolved
.rubocop/strict.yml Show resolved Hide resolved
@mjankowski
Copy link
Contributor Author

mjankowski commented Jun 12, 2024

What is the benefit of adding a binstup for rubocop?

For the binstub aspect specifically here, motivation is some mix of:

  • Regular justification for adding binstubs ... it's just a slightly shorter/elegant shortcut to launch commands wrapped in bundler env load - locally, on CI, etc
  • If you have bin added to $PATH with a .git/safe approach, puts the commands in path as well
  • Rails 7.2 generates rubocop and brakeman binstubs by default (hence Add binstub for brakeman #30493) so its in part an extraction from that other PR

Not earth shattering, just small improvement (we have bin/rails, bin/rspec, bin/rake, etc already)

@ClearlyClaire ClearlyClaire added this pull request to the merge queue Jun 13, 2024
Merged via the queue into mastodon:main with commit 3a191b3 Jun 13, 2024
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants