Add Hash#each_value and Hash#each_key cop #7677

jemmaissroff · 2020-01-28T18:53:57Z

Added a cop to match the style guide's recommendations around hash-each.
This cop enforces correct usage of Hash#each_key and Hash#each_value
as opposed to Hash#keys.each and Hash#values.each.

Before submitting the PR make sure the following are checked:

Wrote good commit messages.
Commit message starts with [Fix #issue-number] (if the related issue exists).
Feature branch is up-to-date with master (if not - rebase it).
Squashed related commits together.
Added tests.
Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

bbatsov · 2020-01-28T19:08:36Z

config/default.yml

+Style/HashEachMethods:
+  Description: 'Use Hash#each_key and Hash#each_value.'
+  StyleGuide: '#hash-each'
+  Enabled: true


Please, change this to pending. We're no longer enabling any new cops by default.

bbatsov · 2020-01-28T19:09:52Z

lib/rubocop/config_loader.rb

@@ -55,7 +55,7 @@ def load_file(file)
      end

      def add_missing_namespaces(path, hash)
-        hash.keys.each do |key|
+        hash.keys.each do |key| # rubocop:disable Style/HashEachMethods


I guess we can just update this, right?

I actually don't think we can in this case. Due to the hash[q] = hash.delete(key) line below, we'll get a RuntimeError: can't add a new key into hash during iteration if we try do a hash.each_key here.

Ops. my bad. Should have read the code more carefully. :-)

Could this whole issue be sidestepped with a

hash.transform_keys { |key| Cop::Cop.qualified_cop_name(key, path) }

? Or do we need to support rubies < 2.5?

bbatsov · 2020-01-28T19:13:14Z

The code looks solid to me. Thanks for tackling this!

I guess we should mark the cop as unsafe, though, as there's definitely some potential for false positives.

jemmaissroff · 2020-01-28T19:19:07Z

The code looks solid to me. Thanks for tackling this!

I guess we should mark the cop as unsafe, though, as there's definitely some potential for false positives.

Thanks for looking, I've pushed another commit where I marked it as unsafe, and also pending!

parkerfinch · 2020-01-28T19:37:21Z

CHANGELOG.md

@@ -99,6 +100,7 @@
 * [#7446](https://github.com/rubocop-hq/rubocop/issues/7446): Add `merge` to list of non-mutating methods. ([@cstyles][])
 * [#7077](https://github.com/rubocop-hq/rubocop/issues/7077): **(Breaking)** Rename `Unneeded*` cops to `Redundant*` (e.g., `Style/UnneededPercentQ` becomes `Style/RedundantPercentQ`). ([@scottmatthewman][])
 * [#7396](https://github.com/rubocop-hq/rubocop/issues/7396): Display assignments, branches, and conditions values with the offense. ([@avmnu-sng][])
+* [#7434](https://github.com/rubocop-hq/rubocop/issues/7434): Fix an incorrect autocorrect for `Style/MultilineWhenThen` when the body of `when` branch starts with `then`. ([@koic][])


Looks like this might be a rebase artifact? The same line appears above in the Bug Fixes section.

Yeah, this change is not the difference in this PR.

parkerfinch · 2020-01-28T19:39:03Z

config/default.yml

+  Description: 'Use Hash#each_key and Hash#each_value.'
+  StyleGuide: '#hash-each'
+  Enabled: pending
+  VersionAdded: '0.79'


Should this be updated to '0.80'? Looks like 0.79 was released already.

koic · 2020-01-29T05:48:00Z

lib/rubocop/cop/style/hash_each_methods.rb

+
+        MSG = 'Use `%<prefer>s` instead of `%<current>s`.'
+
+        def_node_matcher :kv_each, <<-PATTERN


Can you update this entire PR with squiggly heredoc? (<<~)

koic · 2020-01-29T05:51:32Z

spec/rubocop/cop/style/hash_each_methods_spec.rb

+      it 'auto-corrects foo#keys.each with foo#each_key' do
+        new_source = autocorrect_source('foo.keys.each { |k| p k }')
+        expect(new_source).to eq('foo.each_key { |k| p k }')
+      end


Can you use expect_correction for test an automatic correction? The following address will be helpful.
#7650

rrosenblum · 2020-01-31T15:05:22Z

I knew this cop sounded familiar to me. We have Performance/InefficientHashSearch in the Performance project for changing keys.include? and values.include? to key? or has_key? and value? or has_value?. Does this cop make more sense as a Style Cop or a Performance Cop?

I also think we may have tried to implement this same cop in the past. I don't think this change is safe if you are mutating the keys or values while using each_key or each_value.

This code does not work if you change keys.each to each_key because we remove keys
lib/rubocop/config_loader.rb

      def add_missing_namespaces(path, hash)
        hash.keys.each do |key|
          q = Cop::Cop.qualified_cop_name(key, path)
          next if q == key

          hash[q] = hash.delete(key)
        end
      end

Darhazer · 2020-01-31T16:05:37Z

We used to have Performance/HashEach. Not sure what happened to it.

Update: Found the removal of the cop

jemmaissroff · 2020-02-02T13:15:32Z

We used to have Performance/HashEach. Not sure what happened to it.

Update: Found the removal of the cop

Yup, and there was an issue filed where it was suggested to instead by a Style PR. I didn't link it as part of the commit message because it's in a different repo and so I think the link would break. Let me know if I should be doing something differently here!

bbatsov · 2020-02-04T18:09:51Z

@jemmaissroff Just address the failing build and I think we're good here.

bbatsov · 2020-02-04T18:12:25Z

Actually - it might be cool to also make the cop configurable, so that people could decide whether they prefer hash.foo.each or hash.each_foo. Generally we try to be inclusive of most people's preferences. :-) Anyways, that's just an idea - if you don't have time for this it's fine.

rrosenblum · 2020-02-05T13:28:51Z

it might be cool to also make the cop configurable

If we are classifying this as a Style Cop, then I completely agree that it would be cool for this to be configurable. If we were to classify it as a Performance Cop then I don't think the configuration makes sense.

jemmaissroff · 2020-02-05T14:10:08Z

@bbatsov build is fixed!

koic · 2020-02-05T14:22:08Z

.rubocop.yml

@@ -28,6 +28,9 @@ Style/FormatStringToken:
  Exclude:
    - spec/**/*

+Style/HashEachMethods:
+  Enabled: false


This setting looks unnecessary.

I had a build failing that prompted me to add it here. Can you tell me more about why this looks unnecessary?

By default, it is better to use the setting in config/default.yml.

I think I'm getting confused around what pending will do. It appears that breaks builds also. Are you suggesting to leave it as pending and then remove it from here?

Yeah, config/default.yml leaves the specification of Enabled: pending. OTOH, I expect this setting to be removed from .rubocop.yml.

It appears that breaks builds

Can you show me what build error is occurring?

koic · 2020-02-05T14:23:47Z

config/default.yml

+Style/HashEachMethods:
+  Description: 'Use Hash#each_key and Hash#each_value.'
+  StyleGuide: '#hash-each'
+  Enabled: false


Can you update to Enabled: pending status?
#7677 (comment)

koic · 2020-02-05T14:31:44Z

CHANGELOG.md

@@ -8,6 +8,7 @@
 * [#7663](https://github.com/rubocop-hq/rubocop/pull/7663): Add new `Style/HashTransformKeys` and `Style/HashTransformValues` cops. ([@djudd][], [@eugeneius][])
 * [#7619](https://github.com/rubocop-hq/rubocop/issues/7619): Support autocorrect of legacy cop names for `Migration/DepartmentName`. ([@koic][])
 * [#7659](https://github.com/rubocop-hq/rubocop/pull/7659): Layout/LineLength autocorrect now breaks up long lines with blocks. ([@maxh][])
+* Add a cop for `Hash#each_key` and `Hash#each_value`. ([@jemmaissroff][])


Can you add the PR link to this changelog entry?

bbatsov · 2020-02-06T07:58:28Z

It seems you need to rebase to master and run the rake command that regenerates the manual and the build will pass.

jemmaissroff · 2020-02-07T12:26:42Z

Ah okay, that was it! Builds are now passing. Thanks for the help @bbatsov and @koic !

bbatsov · 2020-02-07T12:30:00Z

🎉

Thanks for working on this! 🙇

jemmaissroff mentioned this pull request Jan 28, 2020

[Proposal] Add back minimal HashEachMethods cop rubocop/rubocop-performance#66

Closed

jemmaissroff force-pushed the feature/hash-each-methods branch from cc77cae to 8bb305b Compare January 28, 2020 19:04

bbatsov reviewed Jan 28, 2020

View reviewed changes

bbatsov approved these changes Jan 28, 2020

View reviewed changes

parkerfinch reviewed Jan 28, 2020

View reviewed changes

koic reviewed Jan 29, 2020

View reviewed changes

jemmaissroff force-pushed the feature/hash-each-methods branch from 8f5ce66 to a9381e9 Compare February 5, 2020 13:57

koic reviewed Feb 5, 2020

View reviewed changes

Jemma Issroff added 7 commits February 7, 2020 14:08

Add Hash#each_value and Hash#each_key cops

c01d9ee

Rebased

107a75c

Changed enabled to pending, and marked unsafe

93d0cfc

Updated to use heredoc, and expect_correction

616027e

Fixed broken build

466eddd

Moved in changelog

084c4ae

Next round

d30344d

Next round

1d3bdaf

jemmaissroff force-pushed the feature/hash-each-methods branch from 9f8a426 to 1d3bdaf Compare February 7, 2020 12:12

bbatsov merged commit cca6e8f into rubocop:master Feb 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Hash#each_value and Hash#each_key cop #7677

Add Hash#each_value and Hash#each_key cop #7677

jemmaissroff commented Jan 28, 2020 •

edited

bbatsov Jan 28, 2020

bbatsov Jan 28, 2020

jemmaissroff Jan 28, 2020

bbatsov Jan 28, 2020

parkerfinch Jan 28, 2020

bbatsov commented Jan 28, 2020

jemmaissroff commented Jan 28, 2020

parkerfinch Jan 28, 2020

koic Jan 29, 2020

parkerfinch Jan 28, 2020

koic Jan 29, 2020

koic Jan 29, 2020

koic Jan 29, 2020

rrosenblum commented Jan 31, 2020

Darhazer commented Jan 31, 2020 •

edited

jemmaissroff commented Feb 2, 2020

bbatsov commented Feb 4, 2020

bbatsov commented Feb 4, 2020

rrosenblum commented Feb 5, 2020

jemmaissroff commented Feb 5, 2020

koic Feb 5, 2020

jemmaissroff Feb 5, 2020

koic Feb 5, 2020

jemmaissroff Feb 5, 2020

koic Feb 6, 2020

koic Feb 5, 2020

koic Feb 5, 2020

bbatsov commented Feb 6, 2020

jemmaissroff commented Feb 7, 2020

bbatsov commented Feb 7, 2020


		MSG = 'Use `%<prefer>s` instead of `%<current>s`.'

		def_node_matcher :kv_each, <<-PATTERN

Add Hash#each_value and Hash#each_key cop #7677

Add Hash#each_value and Hash#each_key cop #7677

Conversation

jemmaissroff commented Jan 28, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbatsov commented Jan 28, 2020

jemmaissroff commented Jan 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rrosenblum commented Jan 31, 2020

Darhazer commented Jan 31, 2020 • edited

jemmaissroff commented Feb 2, 2020

bbatsov commented Feb 4, 2020

bbatsov commented Feb 4, 2020

rrosenblum commented Feb 5, 2020

jemmaissroff commented Feb 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbatsov commented Feb 6, 2020

jemmaissroff commented Feb 7, 2020

bbatsov commented Feb 7, 2020

jemmaissroff commented Jan 28, 2020 •

edited

Darhazer commented Jan 31, 2020 •

edited