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

Discrepancy with Ruby Style Guide regarding regexp and BinaryOperatorParameterName cop #8901

Closed
Zajn opened this issue Oct 16, 2020 · 2 comments

Comments

@Zajn
Copy link
Contributor

Zajn commented Oct 16, 2020

I implemented a class which defined =~. I was basing it off a class internal to Rails, and so I also utilized their parameter name of re. In addition to being too short, to my surprise, Rubocop informed me that I violated Naming/BinaryOperatorParameterName and should name my parameter other. I looked at the Rubocop docs, which linked me to the ruby style guide for the other parameter. It is my understanding that Rubocop is based on this guide.

The style guide states:

When defining binary operators and operator-alike methods, name the parameter other for operators with "symmetrical" semantics of operands. Symmetrical semantics means both sides of the operator are typically of same or coercible types.

Operators and operator-alike methods with symmetrical semantics (the parameter should be named other): +, -, *, /, %, **, ==, >, <, |, &, ^, eql?, equal?.

the style guide then states

Operators with non-symmetrical semantics (the parameter should not be named other): <<, [] (collection/item relations between operands), === (pattern/matchable relations).

I would assume that =~ falls under pattern/matchable relations, but I might be wrong. I would expect in this case, that I would not be in violation of the Naming/BinaryOperatorParameterName cop, especially considering =~ is not listed in the operators and operator-alike methods.

For example's sake, here is the aforementioned method that was in violation:

def =~(re)
  re.match?(to_s)
end

Expected behavior

I expected to not be in violation of Naming/BinaryOperatorParameterName for this method.

Actual behavior

Describe here what actually happened.
Please use rubocop --debug when pasting rubocop output as it contains additional information.

For /Users/zach/workspace/checklistpro: configuration from /Users/zach/workspace/checklistpro/.rubocop.yml
configuration from /Users/zach/.gem/ruby/2.7.2/gems/rubocop-performance-1.8.1/config/default.yml
configuration from /Users/zach/.gem/ruby/2.7.2/gems/rubocop-performance-1.8.1/config/default.yml
Default configuration from /Users/zach/.gem/ruby/2.7.2/gems/rubocop-0.93.1/config/default.yml
configuration from /Users/zach/.gem/ruby/2.7.2/gems/rubocop-rails-2.8.1/config/default.yml
configuration from /Users/zach/.gem/ruby/2.7.2/gems/rubocop-rails-2.8.1/config/default.yml
Inheriting configuration from /Users/zach/workspace/checklistpro/.rubocop_todo.yml
Inspecting 1 file
Scanning /Users/zach/workspace/checklistpro/app/models/jetpack_time_zone.rb
C

Offenses:

app/models/jetpack_time_zone.rb:28:10: C: Naming/BinaryOperatorParameterName: When defining the =~ operator, name its argument other.
  def =~(re)
         ^^
app/models/jetpack_time_zone.rb:28:10: C: Naming/MethodParameterName: Method parameter must be at least 3 characters long.
  def =~(re)
         ^^

1 file inspected, 2 offenses detected
Finished in 0.5917779998853803 seconds

Steps to reproduce the problem

Run the following code with ruby hello.rb to verify it works. Then run rubocop hello.rb and verify that the def =~ method is in violation of Naming/BinaryOperatorParameterName.

image

# hello.rb
# frozen_string_literal: true

# Example of Naming/BinaryOperatorParameterName cop
class Hello
  def initialize(name)
    @name = name
  end

  def =~(regex)
    regex.match?('Zach')
  end

  def self.greet(name)
    h = new(name)

    puts 'Hello Zach' if h =~ /#{name}/
  end
end

Hello.greet('Fred')
Hello.greet('Zach')

RuboCop version

~/workspace/rubocop-example » rubocop -V                                                                                                
0.93.1 (using Parser 2.7.2.0, rubocop-ast 0.8.0, running on ruby 2.7.2 x86_64-darwin19)

I would be happy to submit a PR if this is something that should be corrected. If not, thanks for your time!

@marcandre
Copy link
Contributor

marcandre commented Oct 16, 2020

Very good research and reasoning 💪!

I agree this is a false positive for BinaryOperatorParameterName; PRs welcome 😄

@Zajn
Copy link
Contributor Author

Zajn commented Oct 16, 2020

I agree this is a false positive for BinaryOperatorParameterName; PRs welcome 😄

Ok, great! I'd be happy to put one together.

Zajn added a commit to Zajn/rubocop that referenced this issue Oct 16, 2020
…ameterName`

Defining the match operator `=~` triggered a false positive for
`Naming/BinaryOperatorParameterName`, which should not be triggered
for pattern/matchable relations according to the [Ruby style guide](https://rubystyle.guide/#other-arg).
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