Consider enabling some of the pending cops

[ydah]

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]

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]

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]

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

[pirj]

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]

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]

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]

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]

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

[pirj]

I believe so