rubocop / rubocop-rspec

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

RSpec/ImplicitSubject does not allow is_expected on a separate single line #682

Closed obfuscoder closed 6 years ago

obfuscoder commented 6 years ago

I usually write single line tests like so:

it { is_expected.to have_content 'foo bar' }

Sometimes the expression might get too long to fit within the given line length limit. In that case I have to use do/end:

it do
  is_expected.to have_attributes attrib1: 'foo',
                                 attrib2: 'bar'
end

Doing this, however, is not acceptable by RSpec/ImplicitSubject. I am required to name the subject explicitly:

it do
  expected(subname).to have_attributes attrib1: 'foo',
                                       attrib2: 'bar'
end

That is annoying enough, but it is impossible when combining this with rpsec-its:

its(:property) do
  is_expected.to have_attributes attrib1: 'foo',
                                 attrib2: 'bar'
end

I would like this rule to not complain about using is_expected in this scenario.

RuboCop version

0.58.2 (using Parser 2.5.1.2, running on ruby 2.4.1 x86_64-linux)

rubocop-rspec version 1.29.1

I need to disable this rule for the time being.

jsdalton commented 6 years ago

I came here to post the same issue and found the same comment.

The recent addition of implicit cop was inspired by issue #669, where someone was using is_expected in the middle fo multiple logical lines of code, which I think most of us would agree is improper use of the one liner syntax.

However, the examples provided here by @obfuscoder (🙏 ) are exactly the same problem we are running into. Purely for readability (and in some cases to avoid lines that are too long) we break apart a single logical line of code with line breaks.

Having to explicitly name a subject and refactor to use with a named subject solely to silence this bug creates more noise and definitely impairs rather than enhances readability.

I too will simply disable the rule but I would high suggest reconsidering your implementation to count logical vs. actual lines of code.

tom-lord commented 6 years ago

Just encountered exactly this issue too.

I was particularly surprised to discover that its(:foo) { is_expected.to bar } behaves differently to its(:foo) { expect(subject).to bar }, which made me wonder whether this is actually a bug in rspec-its.

But here's the explanation as to why rspec-its doesn't redefine the subject within the block, as you might expect: https://github.com/rspec/rspec-its/issues/4#issuecomment-33904321

Darhazer commented 6 years ago

Thank you for your reports. We will fix the ImplicitSubject cop in the next few days

cheerfulstoic commented 5 years ago

If I understand correctly the fix here was to ignore its blocks, but I don't think that addresses the original concern in this issue: the case where you have a do...end block with just one line inside of it because the one-line version with {...} is too long. Did I miss that fix? I came to this issue looking to suggest that it be possible to ignore multi-line blocks with just one line inside of them

cheerfulstoic commented 5 years ago

(If that's something you'd be down with fixing I can create separate issue)

Darhazer commented 5 years ago

With EnforcedStyle set to single_statement_only it will not report it do ... end block with single expectation

cheerfulstoic commented 5 years ago

Ah, I missed that, sorry! Tried it and it works great, thanks for the quick response!

obfuscoder commented 5 years ago

You might wanna mention single_statement_only in the rubydoc. Only disallow and single_line_only are mentioned there.

pboling commented 5 years ago

single_statement_only is not working for me. All of my single statement specs are being flagged. Should I open a new issue @Darhazer

Darhazer commented 5 years ago

@pboling please post your spec and rubocop configuration, along with the rubocop-rspec version you're using