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

Safe navigator operator (&.) breaks ABC check? #3578

Closed
rachel-carvalho opened this issue Oct 6, 2016 · 2 comments
Closed

Safe navigator operator (&.) breaks ABC check? #3578

rachel-carvalho opened this issue Oct 6, 2016 · 2 comments

Comments

@rachel-carvalho
Copy link

I just noticed that when I added a safe navigator operator (&.) rubocop stopped reporting a method's ABC size. If anything, there should be even more branches, not less.

I updated to the latest version of rubocop and still see the problem.

Fortunately, I was able to reproduce it with bogus code.

With the safe navigator I have no warning, but without it, I get C: Assignment Branch Condition size for why? is too high. [15.07/15], as expected. Snippets are available on the steps to reproduce section.


Expected behavior

It should report C: Assignment Branch Condition size for why? is too high. [15.07/15]

Actual behavior

It finds no issues with the method after I introduce a &..

Steps to reproduce the problem

    1. Run rubocop on this method:
def why?
  @instance1 = other_method
  @instance2 = other_method

  @instance3 = {}

  return if @instance1.blank? || @instance2.blank?

  @instance4 = other_method
  @instance5 = @defined_somewhere_else.method.method

  @instance6 = @instance4.method ? @instance4.other_method : other_method

  @instance7 = @instance6.method { |i| i.method.method }
end
    1. See how it reports C: Assignment Branch Condition size for why? is too high. [15.07/15]
    1. Now add a safe navigator operator on the last line:
def why?
  @instance1 = other_method
  @instance2 = other_method

  @instance3 = {}

  return if @instance1.blank? || @instance2.blank?

  @instance4 = other_method
  @instance5 = @defined_somewhere_else.method.method

  @instance6 = @instance4.method ? @instance4.other_method : other_method

  @instance7 = @instance6&.method { |i| i.method.method }
end
    1. See how it no longer reports a problem.

RuboCop version

0.43.0 (using Parser 2.3.1.4, running on ruby 2.3.1 x86_64-darwin15)
@savef
Copy link
Contributor

savef commented Oct 7, 2016

I think this line is the issue:
https://github.com/bbatsov/rubocop/blob/ec3123f/lib/rubocop/cop/metrics/abc_size.rb#L14

We need to add :csend as an alternate branch node, as per the parser docs.

savef added a commit to savef/rubocop that referenced this issue Oct 7, 2016
This cop wasn't correctly counting method calls that happen with the safe navigation operator (`x&.y`), it has been changed so that they are treated the same as other method calls (like `x.y`).

Adds a test which checks the score of a method with the safe navigation operator. The test would fail without the change in the cop.
@bbatsov bbatsov closed this as completed in d2ba8bf Oct 7, 2016
@rachel-carvalho
Copy link
Author

Thanks so much for such a quick fix 🎉 @savef @bbatsov

Neodelf pushed a commit to Neodelf/rubocop that referenced this issue Oct 15, 2016
…ubocop#3581)

This cop wasn't correctly counting method calls that happen with the safe navigation operator (`x&.y`), it has been changed so that they are treated the same as other method calls (like `x.y`).

Adds a test which checks the score of a method with the safe navigation operator. The test would fail without the change in the cop.
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

No branches or pull requests

2 participants