Add cop to enforce use of `transform_keys` and `transform_values` #7663

djudd-stripe · 2020-01-23T05:11:05Z

Add a Style/HashTransformMethods cop that enforces the use of the Ruby 2.5+ Hash#transform_keys and Hash#transform_values methods in place of patterns that are slower and less readable.

ie, replace:

{a: 1, b: 2}.each_with_object({}) { |(k, v), h| h[k] = 2*v }
{a: 1, b: 2}.map { |k, v| [k.to_s, v] }.to_h

with:

{a: 1, b: 2}.transform_values { |v| 2*v }
{a: 1, b: 2}.transform_keys { |k| k.to_s }

Includes an autocorrect (which is not safe because we can't fully guarantee that the receiver is a Hash, rather than some other enumerable, but which empirically is fairly good in Stripe's codebase).

This cop should only be run on Ruby 2.5 or greater.

buehmann · 2020-01-23T05:31:15Z

See https://github.com/rubocop-hq/rubocop/pull/7646/files for an example of how to specify minimum_target_ruby_version

djudd-stripe · 2020-01-23T16:07:18Z

Added minimum_target_ruby_version - thanks @buehmann!

config/default.yml

lib/rubocop/config.rb

CHANGELOG.md

lib/rubocop/cop/style/hash_transform_methods.rb

bbatsov · 2020-01-26T14:59:36Z

Nice idea!

I've added a few small remarks and I'd suggest also adding a matching guideline in the community Ruby style guide.

You'll also have to get the build to pass. :-)

djudd-stripe · 2020-01-26T18:32:03Z

Thanks for the feedback! I think I've responded to everything either by fixing or with a question.

eugeneius

I was just about to submit a very similar PR to the performance extension when I found this 😅

I left a few notes based on my own implementation. Feel free to plunder anything else valuable:

rubocop/rubocop-performance@master...eugeneius:hash_transformation

lib/rubocop/cop/style/hash_transform_methods.rb

.rubocop.yml

spec/rubocop/cop/style/hash_transform_methods_spec.rb

lib/rubocop/cop/style/hash_transform_methods.rb

bbatsov · 2020-01-27T07:04:04Z

Btw, I've been thinking that probably we need to have 2 different cops (e.g. HashTransformKeys and HashTransformValues), as this is going to simplify the code base IMO - one for transform_keys and one for transform_values. I know the two methods are quite related, but there are also a bunch of differences between them and the two methods were introduced in different Ruby versions (2.4 and 2.5). Having two cops would give more flexibility to people targeting Ruby 2.4. There can still be some shared class for the common logic needed in both cops.

djudd-stripe

@bbatsov I thought about that but decided not to bother since EOL for Ruby 2.4 is coming up in a couple of months--and I agree we could factor out a lot of common code between the two, but it still seems likely to end up a more complex implementation to me rather than simpler.

If you think it's important I can separate them, but now that the weekend is over I'm not sure when I'll get to it; if @eugeneius has time to iterate on their branch it might make sense to use that as the base and steal anything valuable from this (probably mostly the each_with_object case?) instead of vice versa.

djudd-stripe · 2020-02-03T01:11:48Z

@bbatsov I did end up getting some time to work on this today, so I've pushed a split into two cops (and also fixed the autocorrect bug @eugeneius mentioned)

bbatsov · 2020-02-04T18:07:15Z

Thanks!

djudd-stripe · 2020-02-04T18:16:25Z

Thank you!

This PR suppress the following warning. ```console % cd path/to/rubocop % bundle install Your Gemfile lists the gem pry (>= 0) more than once. You should probably keep only one of them. Remove any duplicate entries and specify the gem only once. While it's not a problem now, it could cause errors if you change the version of one of them later. Using rake 12.3.3 Using public_suffix 4.0.3 (snip) ``` `pry` gem is already in the Gemfile. This looks like it was accidentally added in rubocop#7663.

This PR suppress the following warning. ```console % cd path/to/rubocop % bundle install Your Gemfile lists the gem pry (>= 0) more than once. You should probably keep only one of them. Remove any duplicate entries and specify the gem only once. While it's not a problem now, it could cause errors if you change the version of one of them later. Using rake 12.3.3 Using public_suffix 4.0.3 (snip) ``` `pry` gem is already in the Gemfile. This looks like it was accidentally added in #7663.

bquorning · 2020-02-19T08:54:37Z

lib/rubocop/cop/mixin/hash_transform_method.rb

+      end
+
+      def handle_possible_offense(node, match, match_desc)
+        puts node.class


Perhaps we should have a cop for catching puts? 😄

Wow! For example, it would be good to prepare a cop of InternalAffairs department for lib/rubocop/cop directory. At this time, puts method is not used in this directory.

This PR added `VersionAdded` to `Style/HashTransformKeys` and `Style/HashTransformValue`. This Cops was added at rubocop#7663 and published at 0.80.0.

sebastian-palma · 2020-06-10T10:07:33Z

Hey @eugeneius, I just noticed that your branch makes mention and adds a cop to use Hash#to_h instead of Hash#map...to_h or Hash[Hash#map], but this PR covers _.each_with_object({}) {...}, _.map {...}.to_h, and Hash[_.map {...}] with no mention of the possibility of passing a block toto_h.

Will you keep working on yours?

djudd-stripe force-pushed the hash-transform-methods-cop branch from 908dcbb to 1e4b1d0 Compare January 23, 2020 16:06