rubocop / rubocop-rspec

Code style checking for RSpec files.
https://docs.rubocop.org/rubocop-rspec
MIT License
794 stars 272 forks source link

Enable pending cops up to RuboCop v1.63 #1879

Closed bquorning closed 1 month ago

bquorning commented 2 months ago

I enabled the pending cops up until RuboCop v1.63, and fixed most offenses. We want to keep the NewCops: disable settings, so PRs are not blocked by newly added cops forcing changes to the entire code base. But we should update once in a while, even if it means everyone needs to bundle update. Or we could add rubocop to Gemfile with a version restriction?

I am fixing Performance/MethodObjectAsBlock in two commits, which should be squashed before merging:

  1. Fix Performance/MethodObjectAsBlock using numbered parameters: I really dislike the foo(&method(:bar)) style, and in my opinion Performance/MethodObjectAsBlock might as well have been a Style cop. I tried fixing the offenses using numbered parameters (_1) but I‘m not a fan of that syntax either, to be honest.
  2. Fix Performance/MethodObjectAsBlock using block arguments: I think the plain old block arguments are much easier to read than numbered parameters – except maybe in a few cases. E.g. is { |n| let?(n) } really better to read than { let?(_1) }?

The Lint/ToEnumArguments offense listed in the todo file is due to a bug: rubocop/rubocop#12885


Before submitting the PR make sure the following are checked:

bquorning commented 1 month ago

cc @pirj I would like your opinion on this PR too.

bquorning commented 1 month ago

I’d rather keep it as is, and run our the code in specs with the latest released (or latest cached by bundler?) RuboCop version rather than inspected our code with it.

I am not sure I understand this part of your response. Can you please elaborate a bit?

I think the three conflicting goals here are:

  1. We want to use new cops pretty soon after they are released,
  2. we don’t want to require an unnecessarily recent version of RuboCop in our gemspec, and
  3. we don’t want pull requests to fail CI because of a new RuboCop version (so no using edge version on CI).
pirj commented 1 month ago

There is one thing we might be missing - it’s to test our code against our minimum specified rubocop version. Imagine, we have 1.40 in our min, 1.41 introduced some nice helper, 1.42 added an internal investigation cop to enforce the use of this helper, we started using it (because our goal is to inspect our code with the latest rubocop), and our code breaks for our users who updated rubocop-rspec, but stick to rubocop 1.40.

pirj commented 1 month ago

Please correct me, but it feels that 1 is not easy together with 3. If we have a few PRs open and enable pending cops, this has a good chance of either:

I don’t have a good solution to this, and think it’s inevitable. If we don’t enable new cops too often -it’s not a big burden.

bquorning commented 1 month ago

Yeah, you’re probably right. We should not enable new cops very often.

And we should start running CI with the minimum allowed RuboCop version.

bquorning commented 1 month ago

There is one thing we might be missing - it’s to test our code against our minimum specified rubocop version.

See #1882.

bquorning commented 1 month ago

Rebased. Are you ok with merging @pirj ?

pirj commented 1 month ago

Yes, please proceed with the merge. All perfect. Thank you!