Style/LegacyHash cop, prohibiting Hash[] method #9478

zverok · 2021-01-30T13:13:01Z

# bad
Hash[ary]
^^^^^^^^^ Prefer ary.to_h to Hash[ary].

# good
ary.to_h

# bad
Hash[a, b, c, d]
^^^^^^^^^^^^^^^^ Prefer literal hash to Hash[arg1, arg2, ...].

# good
{a => b, c => d}

# bad
Hash[]
^^^^^^ Prefer literal hash to Hash[arg1, arg2, ...].

# good
{}

Correctly identifies un-autocorrecable cases (Hash[*foo], and an odd number of arguments).
I am open to suggestions regarding the name, it is my best guess :)

Before submitting the PR make sure the following are checked:

The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
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.
Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

tejasbubane · 2021-01-30T15:03:27Z

CHANGELOG.md

+### New features
+
+* [#9478](https://github.com/rubocop-hq/rubocop/pull/9478): Add new `Style/LegacyHash` cop. ([@zverok][])
+


Please refer this for creating changelog entries. It will create separate files under /changelog which will later be combined into this main file by rubocop maintainer during release.

tejasbubane · 2021-01-30T15:18:32Z

lib/rubocop/cop/style/legacy_hash.rb

+
+        MSG_TO_H = 'Prefer ary.to_h to Hash[ary].'
+        MSG_LITERAL = 'Prefer literal hash to Hash[arg1, arg2, ...].'
+        MSG_SPLAT = 'Prefer array_of_pairs.to_h to Hash[*array].'


These messages should be have dynamic variable names. Please refer to other cops like this one

I contemplated this, but I prefer not to. The real code that caused offense might look like this (Rubocop's own codebase):

Hash[PERCENT_LITERAL_TYPES.map do |type| [type, preferred_delimiters_config[type] || preferred_delimiters_config['default']] end]

...and that's not the limit, block inside Hash[] can take many lines, so the suggestion would look really cumbersome. That's why I've decided to stick with abstract recommendations.

@zverok that's a fair point. I think maybe one case we can be more explicit about is Hash[] to {}.

@dvandersluis I actually believe it is the tiniest percent of actual Hash::[] usages in code, and didn't want to complicate the code for the rare case.

Fair enough, I was basing on your example cases.

tejasbubane · 2021-01-30T15:19:45Z

lib/rubocop/cop/style/legacy_hash.rb

+
+        def single_argument(node)
+          if node.arguments.first.splat_type?
+            add_offense(node, message: MSG_SPLAT)


We should auto-correct this too.

I don't think so. With splat, there are basically two possibilities:

Code inside Hash[] produces a flat list of keys and values by design (say, it reads a file which has [key, value, key, value] split line-by-line)

Code actually produces an array of pairs, and then flatten(1) added to the end of it to match Hash[] signature.

In both cases, it should be reviewed and redecided by human.
The only working suggestion we can produce is

- Hash[*something] + something.each_slice(2).to_h

...and in most cases, it would not be a good suggestion.

tejasbubane · 2021-01-30T15:24:37Z

spec/rubocop/cop/style/legacy_hash_spec.rb

+    RUBY
+  end
+
+  it 'does not tries to correct multi-argument Hash with odd number of arguments' do


For wordings of these example descriptions, please refer: https://github.com/rubocop-hq/rubocop/blob/77db90e6fa0f45ea7cb0b6ec4bee23faf66a385a/lib/rubocop/cop/internal_affairs/example_description.rb

I don't understand this suggestion, sorry. What exactly should be fixed here (except for "tries"→"try" 😂)?

Yeah after correcting "tries" I think it's fine. The internal affairs cop is just to make sure that we don't have descriptions that contradict the actual test.

config/default.yml

lib/rubocop/cop/style/legacy_hash.rb

bbatsov · 2021-01-30T17:17:10Z

It seems to me that a name like ArrayToHashConversation or something along those lines would be better, as it's both more neutral (allows us to make this configurable) and more descriptive of what the cop actually checks for.

zverok · 2021-01-30T17:23:02Z

@bbatsov This was my first guess, but...

Hash[:a, 1, :b, 2]

...has no array to be seen :) (And we really have a few of those in the codebase, so they can happen in the wild!)

I thought about configurability and (maybe too bold for me) decided there should be no reason to prefer Hash[foo] to foo.to_h, the .to_h is clearly superior.

bbatsov · 2021-01-30T17:27:43Z

Fair points. Maybe simply HashConversion then? I still think that LegacyHash is not the most informative name.

zverok · 2021-01-30T17:43:40Z

@bbatsov Yeah, that's ideal.
Updated cop name, edited commit history to make more sense, pweeze check!

bbatsov

Looks great. I'll merge it after we cut 1.9.1.

jdufresne · 2021-01-31T15:50:44Z

lib/rubocop/cop/mixin/preferred_delimiters.rb

+              PERCENT_LITERAL_TYPES.map do |type|
                [type, preferred_delimiters_config[type] ||
                  preferred_delimiters_config['default']]
-              end]
+              end.to_h


I believe this could be further simplified to avoid the map

PERCENT_LITERAL_TYPES.to_h do |type| [type, preferred_delimiters_config[type] || preferred_delimiters_config['default']] end

to_h with a block was only added in ruby 2.6, but RuboCop supports 2.4, so we can’t use that.

Ah I see. Thanks for explaining that.

bbatsov · 2021-02-01T11:00:12Z

@zverok Can you squash the fixup commit?

zverok · 2021-02-01T11:09:14Z

@bbatsov Done

bbatsov · 2021-02-04T13:54:50Z

changelog/new_style_hash_conversion_cop.md

@@ -0,0 +1 @@
+* [#9478](https://github.com/rubocop-hq/rubocop/pull/9478): Add new `Style/LegacyHash` cop. ([@zverok][])


Just noticed that we need to update the name here as well.

@bbatsov fixed

* Update rubocop from 1.12.1 to [1.13.0](https://github.com/rubocop-hq/rubocop/releases/tag/v1.13.0) * Update rubocop-performance from 1.9.2 to [1.11.1](https://github.com/rubocop-hq/rubocop-performance/releases/tag/v1.11.1) * Enabled the following rules: * [`Performance/RedundantSplitRegexpArgument`](rubocop/rubocop-performance#190) * [`Style/IfWithBooleanLiteralBranches`](rubocop/rubocop#9396) * [`Lint/TripleQuotes`](rubocop/rubocop#9402) * [`Lint/SymbolConversion`](rubocop/rubocop#9362) * [`Lint/OrAssignmentToConstant`](rubocop/rubocop#9363) * [`Lint/NumberedParameterAssignment`](rubocop/rubocop#9326) * [`Style/HashConversion`](rubocop/rubocop#9478) * [`Gemspec/DateAssignment`](rubocop/rubocop#9496) * [`Style/StringChars`](rubocop/rubocop#9615)

zverok force-pushed the hash-to_h branch 2 times, most recently from e2e7a13 to b2022f7 Compare January 30, 2021 13:18

zverok mentioned this pull request Jan 30, 2021

Hash[] vs to_h #9460

Closed

tejasbubane reviewed Jan 30, 2021

View reviewed changes

dvandersluis reviewed Jan 30, 2021

View reviewed changes

config/default.yml Outdated Show resolved Hide resolved

dvandersluis reviewed Jan 30, 2021

View reviewed changes

lib/rubocop/cop/style/legacy_hash.rb Outdated Show resolved Hide resolved

dvandersluis reviewed Jan 30, 2021

View reviewed changes

lib/rubocop/cop/style/legacy_hash.rb Outdated Show resolved Hide resolved

zverok force-pushed the hash-to_h branch from 6ac7a19 to 30d5d1b Compare January 30, 2021 17:14

zverok force-pushed the hash-to_h branch from 30d5d1b to f67c57d Compare January 30, 2021 17:42

bbatsov approved these changes Jan 31, 2021

View reviewed changes

jdufresne reviewed Jan 31, 2021

View reviewed changes

zverok force-pushed the hash-to_h branch 2 times, most recently from a1a15a3 to a86c06b Compare February 1, 2021 11:08

dvandersluis linked an issue Feb 1, 2021 that may be closed by this pull request

Hash[] vs to_h #9460

Closed

bbatsov reviewed Feb 4, 2021

View reviewed changes

zverok added 2 commits February 4, 2021 15:57

[Fix rubocop#9460] Introduce Style/HashConversion cop, guarding Hash[]

6b4ee8e

Auto-correct codebase with the new cop

0acf09d

zverok force-pushed the hash-to_h branch from a86c06b to 0acf09d Compare February 4, 2021 14:05

bbatsov merged commit edef5e2 into rubocop:master Feb 4, 2021

kevindew mentioned this pull request Feb 16, 2021

Update rubocop requirement from ~> 1.7.0 to ~> 1.10.0 alphagov/rubocop-govuk#127

Merged

jmkoni mentioned this pull request May 3, 2021

Update rubocop and rubocop-performance and finally add in some new cops standardrb/standard#287

Merged

zverok deleted the hash-to_h branch May 5, 2021 20:40

cllns mentioned this pull request Jul 1, 2021

Don't prohibit Hash[] syntax hanami/devtools#69

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Style/LegacyHash cop, prohibiting Hash[] method #9478

Style/LegacyHash cop, prohibiting Hash[] method #9478

zverok commented Jan 30, 2021 •

edited

Loading

tejasbubane Jan 30, 2021 •

edited

Loading

tejasbubane Jan 30, 2021

zverok Jan 30, 2021

dvandersluis Jan 30, 2021

zverok Jan 30, 2021

dvandersluis Jan 30, 2021

tejasbubane Jan 30, 2021

zverok Jan 30, 2021

tejasbubane Jan 30, 2021

zverok Jan 30, 2021

dvandersluis Jan 30, 2021

bbatsov commented Jan 30, 2021

zverok commented Jan 30, 2021

bbatsov commented Jan 30, 2021

zverok commented Jan 30, 2021

bbatsov left a comment

jdufresne Jan 31, 2021

dvandersluis Jan 31, 2021

jdufresne Jan 31, 2021

bbatsov commented Feb 1, 2021

zverok commented Feb 1, 2021

bbatsov Feb 4, 2021

zverok Feb 4, 2021

		### New features

		* [#9478](https://github.com/rubocop-hq/rubocop/pull/9478): Add new `Style/LegacyHash` cop. ([@zverok][])

		@@ -0,0 +1 @@
		* [#9478](https://github.com/rubocop-hq/rubocop/pull/9478): Add new `Style/LegacyHash` cop. ([@zverok][])

Style/LegacyHash cop, prohibiting Hash[] method #9478

Style/LegacyHash cop, prohibiting Hash[] method #9478

Conversation

zverok commented Jan 30, 2021 • edited Loading

tejasbubane Jan 30, 2021 • edited Loading

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

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 30, 2021

zverok commented Jan 30, 2021

bbatsov commented Jan 30, 2021

zverok commented Jan 30, 2021

bbatsov left a 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 1, 2021

zverok commented Feb 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zverok commented Jan 30, 2021 •

edited

Loading

tejasbubane Jan 30, 2021 •

edited

Loading