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

Consider enabling some of the pending cops #1886

Closed
bquorning opened this issue May 21, 2024 · 10 comments · Fixed by #1902
Closed

Consider enabling some of the pending cops #1886

bquorning opened this issue May 21, 2024 · 10 comments · Fixed by #1902
Milestone

Comments

@bquorning
Copy link
Collaborator

No description provided.

@bquorning bquorning added this to the 3.0 milestone May 21, 2024
@ydah
Copy link
Member

ydah commented May 21, 2024

IMO, basically, I think there is no problem with all pending cops change to enable. However, we should consider disabling it if there are many people who want to disable it due to Issues.

@pirj
Copy link
Member

pirj commented May 21, 2024

disable it due to Issues

The only case I can justify for myself to disable a cop is if it’s extremely buggy.
So buggy that it would take until the next major version to fix.

If it’s just buggy, we should fix it (or, better help our contributors fix it).

I’m against marking cops as unsafe or unsafe for autocorrection if they are buggy. This is a an example of good cops going on retirement effectively (because nobody runs -A) just because of a bug that noone committed to fixing.
I was going to start a discussion about this in rubocop, but went lazy.

My suggestion:

  1. Check pending cops’ Safe and SafeAutocorrect. If cops’ doc doesn’t have a section about why it’s unsafe, or there’s no such discussion anywhere, or the explanation of unsafety is due to a bug, not a as-by-design unsafe (like eg the order of hooks).
  2. Fix bugs of pending cops that are supposed to be safe by design (and mark them as safe)
  3. Enable all pending cops

We’ll have feedback from a wider audience than previously (our early adopters with NewCops: true), might be a lot to handle. But we don’t force anyone to upgrade to the new major version, and we’ll have time to release bugfix 3.0.x versions.

The very purpose of releasing a major version is to enable all pending cops, and bravely get all the feedback.

@bquorning
Copy link
Collaborator Author

Of the currently pending cops, it’s only RSpec/ContainExactly and RSpec/IndexedLet that I don’t have enabled on my work project.

RSpec/IndexedLet probably mostly because we have almost 400 offenses and I consider that a lost cause at this point 😅 I can certainly see the cop having its use on smaller projects.

As for RSpec/ContainExactly, this comment resonated with me:

My take away is that match_array(['x', 'y']))created a little more garbage and some unneeded characters.

You could also argue that we should so contain_exactly(*[1,2,3]) since this is what match_array method does http://rspec.info/documentation/3.12/rspec-expectations/RSpec/Matchers.html#match_array-instance_method

(source code)

I’m fine with people using match_array instead of contain_exactly in some cases, but I’m not sure I’m comfortable that that we start enforcing it by default.

@bquorning
Copy link
Collaborator Author

@pirj, @ydah what are you thoughts on not enabling RSpec/ContainExactly in v3?

@pirj
Copy link
Member

pirj commented May 30, 2024

We have MatchArray (that pushes towards contains_exactly), and ContainExactly (that pushes for match_array usage), but they are not mutually exclusive, are they? We can see how they play together, and adjust accordingly if they clash?

@bquorning
Copy link
Collaborator Author

I have no problem with the MatchArray cop; all the corrections it told me to do were spot on. I just don’t see the value in the ContainExactly cop telling me to change from

expect(foo).to contain_exactly(*arr_a, *arr_b)

to

expect(foo).to match_array(arr_a + arr_b)

Some of the corrections are fine with me though, e.g. when splatting a literal array: contain_exactly(*[1, 2, 3])match_array([1, 2, 3]).

@pirj
Copy link
Member

pirj commented Jun 3, 2024

I think this goes in line with the recent additions to rubocop-performance which started recommending arr1 + arr2 instead of [*arr1, *arr2]. I have no idea why, just can guess that because of performance.

Speaking of the other example, can’t it just be contain_exactly(1, 2, 3)? 🤔

@bquorning
Copy link
Collaborator Author

Speaking of the other example, can’t it just be contain_exactly(1, 2, 3)? 🤔

Actually, Lint/RedundantSplatExpansion would change contain_exactly(*[1, 2, 3]) to contain_exactly(1, 2, 3).

But if we ignore that for a second, then RSpec/ContainExactly would have changed it into match_array([1, 2, 3]), and then RSpec/MatchArray would change it back into contain_exactly(1, 2, 3).

@bquorning
Copy link
Collaborator Author

So, should we just mark RSpec/ContainExactly as enabled? I’ll prepare a pull request.

@bquorning bquorning mentioned this issue Jun 3, 2024
6 tasks
@pirj
Copy link
Member

pirj commented Jun 3, 2024

I believe so

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 a pull request may close this issue.

3 participants