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

Remove reference to performance cop #6

Conversation

dylnclrk
Copy link
Member

Performance cops were moved out in to their own gem in Rubocop 0.68, which
means that referring to them in our default .rubocop.yml will log a warning like,

Warning: unrecognized cop Performance/Casecmp found in .rubocop-https---raw-githubusercontent[…]

when rubocop is run.

To resolve this we can either:

  • Remove the reference to the performance cop.
  • Add the new gem rubocop-performance as a dependency in Raygun, and add require: rubocop-performance to our default .rubocop.yml.

Removing the reference seems like the lower impact change, but it may negatively impact users who do not upgrade to 0.68, as this previously disabled Performace/Casecmp cop will now be enabled until they upgrade to 0.68.

Performance cops were moved out in to their own gem in Rubocop 0.68[1], which
means that referring to them logs a warning like:

`Warning: unrecognized cop Performance/Casecmp found in …`

Remove the reference to the performance cop to resolve this. Note that it may
negatively impact users who do not upgrade to 0.68, as this previously disabled
Performace cop will now be enabled until they upgrade to 0.68.

[1]: rubocop/rubocop#5977
@mattbrictson
Copy link
Contributor

I am in favor of this PR to just remove Performance/Casecmp for now. I find the code that this cop suggests is hard to read anyway 🙂

# Performance/Casecmp

# bad
str.downcase == 'abc'
str.upcase.eql? 'ABC'
'abc' == str.downcase
'ABC'.eql? str.upcase
str.downcase == str.downcase

# good
str.casecmp('ABC').zero?
'abc'.casecmp(str).zero?

casecmp(str).zero? is really awkward, IMHO

@mattbrictson
Copy link
Contributor

Oops, I just realized disabling the cop is the whole point of this part of the conventions file. 🤦‍♂

A vote from me not to merge, then.

@dylnclrk
Copy link
Member Author

dylnclrk commented Apr 30, 2019

Closing this! (in favor of the other approach)

@dylnclrk dylnclrk closed this Apr 30, 2019
@dylnclrk dylnclrk deleted the chores/remove-performance-cop-references branch April 30, 2019 23:16
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 this pull request may close these issues.

None yet

2 participants