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

[Proposal] Add back minimal HashEachMethods cop #66

Closed
JacobEvelyn opened this issue Jul 1, 2019 · 6 comments
Closed

[Proposal] Add back minimal HashEachMethods cop #66

JacobEvelyn opened this issue Jul 1, 2019 · 6 comments

Comments

@JacobEvelyn
Copy link
Contributor

JacobEvelyn commented Jul 1, 2019

Is your feature request related to a problem? Please describe.

I recently noticed that the Performance/HashEachMethods cop has been removed, and reading that commit message and those of other commits it seems it comes down to the fact that it had a lot of false positives for negligible performance gains when trying to correct hash.each { |k, _| ... } to hash.each_key { |k| ... }.

I agree that that portion of the cop wasn't very useful. However, the cop also checked for another case: hash.keys.each { |k| ... } or hash.values.each { |v| ... } instead of the more performant hash.each_key { |k| ... } or hash.each_value { |k| ... }.

I miss that check in my code and would love to bring it back. Even though they're not quite equivalent—hash.keys.each {} will return an array of keys—at our organization we'd almost never use hash.keys.each in ways where we couldn't change it to each_key, and I think it would be fine to use an inline # rubocop:disable Performance/HashEachMethods in the rare exceptions.

Describe the solution you'd like

I'd love to bring back the Performance/HashEachMethods cop, but only the check for keys.each and values.each, and not the more complex code that inferred block argument usage and tended to produce false positives.

Describe alternatives you've considered

If it alleviates confusion we could make a new cop instead, but I actually think that would be less helpful since there are old websites/StackOverflow answers/etc. that refer to the old cop by name and describe it checking for .keys.each and .values.each.

Additional context

It seems like this check is still desired since it's still a part of the Rubocop ruby style guide: https://github.com/rubocop-hq/ruby-style-guide#hash-each

If I'm misunderstanding something about why it was removed, please let me know!

@JacobEvelyn JacobEvelyn changed the title Add back minimal HashEachMethods cop? [Proposal] Add back minimal HashEachMethods cop? Jul 16, 2019
@JacobEvelyn JacobEvelyn changed the title [Proposal] Add back minimal HashEachMethods cop? [Proposal] Add back minimal HashEachMethods cop Jul 16, 2019
@JacobEvelyn
Copy link
Contributor Author

@koic is there another way I should be advocating for this change?

@bbatsov
Copy link
Contributor

bbatsov commented Oct 21, 2019

With a pull request. :-) Generally I'm not opposed to what you're suggesting, although it's debatable if that'd be a performance cop or some style cop. Perhaps in the past we should have just disabled it by default.

@JacobEvelyn
Copy link
Contributor Author

@bbatsov thanks. Would you recommend this being a performance or style cop?

@jemmaissroff
Copy link

I'm working on a PR here and will plan to do it in https://github.com/rubocop-hq/rubocop as a style cop. Would that make sense to you @JacobEvelyn and @bbatsov?

@jemmaissroff
Copy link

PR is up here rubocop/rubocop#7677! cc @JacobEvelyn @bbatsov. Full disclosure, this is my first open source PR so please advise me if I did anything incorrectly.

@tejasbubane
Copy link
Contributor

I think this issue can be closed since a new cop has been added to rubocop.

@koic koic closed this as completed Apr 20, 2020
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

5 participants