New hash syntax cop that reports mixed syntax in the same hash #1641

iainbeeston · 2015-02-06T15:24:39Z

When I'm working on legacy code I don't care if hashes use hash rocket syntax or 1.9 syntax, but I do dislike hashes that mix the two. For example, I often see hashes that look like this {a: 1, :b => 2} (note that it mixes both styles of hash key). I usually end up converting these to use the same style, so that the previous example would become {a: 1, b: 2}. But I don't especially care which style I use (hash rockets or 1.9 style) and I don't want to change all of the hashes throughout a project to use a uniform style. My preference would be to change as few entries as possible (in a given hash) until they all use the same style, so for example {:a => 1, :b => 2, c: 3} I would change to {:a => 1, :b => 2, :c => 3}.

This PR adds a new cop that reports only hashes that mix the two forms, and autocorrects them to use a unified style (based on whichever style is most common in a given hash)

iainbeeston · 2015-02-06T15:29:24Z

lib/rubocop/cop/style/hash_syntax.rb

+                if style == :dont_mix
+                  unrecognized_style_detected
+                else
+                  alternative_style = style == :ruby19 ? 'hash_rockets' : 'ruby19'


Not sure what the best way to do this is... Really I'm trying to preserve the behaviour of opposite_style_detected (which expects 2 styles but here there are 3)

iainbeeston · 2015-02-06T15:29:45Z

Also, I wasn't sure what to put in the changelog for this... "Updated cop"?

jdickey · 2015-02-06T15:41:03Z

If you're going to have this, it also needs to respect cases where the 1.9 syntax either doesn't work or seriously degrades readability.

A trivial example, taken from one of my current RSpec specs:

  let(:repo_failure_result) do
    FancyOpenStruct.new entity: nil, :success? => false
  end

What is the 1.9-proper way to do this?

iainbeeston · 2015-02-06T16:01:54Z

I don't think I understand your question. Are you referring to the autocorrection defaulting to ruby 1.9 syntax? (Which is an arbitrary choice - I don't feel strongly either way)

In your case I would expect the new style to report an offence, and encourage you to use a consistent style. Personally here I'd convert it to FancyOpenStruct.new :entity => nil, :success? => false.

jdickey · 2015-02-07T19:18:40Z

@iainbeeston Meh; that's sensible, I guess. If the project default is to use 1.9 syntax, then hash-rockets should alert the reader to an "oddly-formed" key being used (that's easy to mistype; :success? vs :success here). I've far less of a problem with the idea of this cop now than when I wrote the earlier comment. Thanks.

iainbeeston · 2015-02-07T20:05:23Z

I think I might be doing a bad job of explaining what this new style does. It doesn't enforce either hash rockets or 1.9 style hashes throughout a project. It looks at each hash as an individual case. If there are some hashes using 1.9 syntax and some using rockets, that's fine, so long as no hashes mix the two (in the same hash). It's only mixed hashes that are reported (eg. {a: 1, :b => 2} - which uses both 1.9 style and hash rockets).

Incidentally I think I need to change the behaviour of the auto-correct mode, such that if it finds a hash that uses both styles (eg. {foo: 'bar', :hello => 'world', :why => 'not'}) then it converts it to use whichever style is most common in that hash (eg. in the previous case that would be {:foo => 'bar', :hello => 'world', :why => 'not'}) or 1.9 style in the case of a tie (where both are equally common).

iainbeeston · 2015-02-08T08:29:21Z

Ok, I've added a more intelligent autocorrect and completed the changelog now.

Hopefully this is good enough to merge

coveralls · 2015-02-08T08:36:28Z

Coverage decreased (-0.07%) to 99.45% when pulling 1a7af4f0e6d5bbfdd5cf4932144b8e2212a152f0 on iainbeeston:dont_mix_hash_syntax into 03cc933 on bbatsov:master.

coveralls · 2015-02-08T08:36:29Z

Coverage increased (+0.12%) to 99.65% when pulling 1a7af4f0e6d5bbfdd5cf4932144b8e2212a152f0 on iainbeeston:dont_mix_hash_syntax into 03cc933 on bbatsov:master.

coveralls · 2015-02-08T08:40:24Z

Coverage increased (+0.12%) to 99.65% when pulling fb1d8bfe1513e06cdc767d98381db33ae5766aae on iainbeeston:dont_mix_hash_syntax into 03cc933 on bbatsov:master.

iainbeeston · 2015-02-13T13:24:39Z

Can I get any feedback on this?

bbatsov · 2015-02-13T14:08:18Z

@iainbeeston Sorry, mate. I've been super busy at work, but I'll have a look at the PR now.

bbatsov · 2015-02-13T14:10:12Z

config/default.yml

@@ -304,6 +304,7 @@ Style/HashSyntax:
  SupportedStyles:
    - ruby19
    - hash_rockets
+    - dont_mix


Not sure about this name, but I can't suggest a better option right away.

Maybe:

unmixed

consistent

uniform

None of those are great... I'll keep thinking about it. Better to get that
right before merging and being stuck with it.

iainbeeston · 2015-02-13T14:10:25Z

Hey no worries! It's not urgent.

iainbeeston · 2015-02-13T14:10:49Z

But thanks

bbatsov · 2015-02-13T14:19:44Z

The code looks good to me, but I wonder how many people are interested in this style. Allowing the mixing of 1.9 and hash-rocket syntax in the same codebase is definitely not a popular practice.

I generally avoid introducing support for styles that aren't widely adopted.

iainbeeston · 2015-02-13T14:22:55Z

That is a valid point. It's less about defining a consistent style for a code-base than a pragmatic choice (when you can't go ahead and change everything). I'll ask around and see if I know anyone else who likes to work this way.

bbatsov · 2015-02-13T14:24:17Z

OK. That'd be great.

P.S. #1589 also deals with alternative hash literal styles.

iainbeeston · 2015-02-13T14:26:32Z

Wow I really don't understand that style. I guess that's probably how this PR looks to others...

iainbeeston · 2015-02-13T14:57:34Z

I've talked to a few people, and maybe I'm approaching this problem the wrong way.

The 1.9 style cop already ignores hash rockets if a particular key can't be expressed in 1.9 style. That's great, but what I'm really looking for is that but also not allowing styles to be mixed. So that if a hash has any keys that can't be expressed in 1.9 style, then all of the keys in the hash should be in hash rocket style instead. For example, if we have { :"1" => 1, a: 2 } right now that's fine with the 1.9 style cop. But I'd prefer for it to be considered an offence, and rubocop suggest (or autocorrect) { :"1" => 1, :a => 2 } instead (because then it's not mixing styles).

I'm not sure if this is desirable in rubocop though, and even if it is, whether this should be a distinct style or a flag you can set on the 1.9 style.

bbatsov · 2015-02-14T09:20:34Z

I'm not sure if this is desirable in rubocop though, and even if it is, whether this should be a distinct style or a flag you can set on the 1.9 style.

I think making this a flag for 1.9 is pretty sensible. A new style just for this tiny difference would an overkill IMO.

iainbeeston · 2015-02-14T10:34:05Z

Thanks for the feedback. Can I ask though: what's the correct way to set a flag on a particular style? Are there any other examples you can point me towards?

bbatsov · 2015-02-14T11:20:15Z

Thanks for the feedback. Can I ask though: what's the correct way to set a
flag on a particular style? Are there any other examples you can point me
towards?

Not sure if we've done this at all so far. Perhaps a good idea would be to prefix the flag with the name of the style - e.g. Ruby19AllowMixedPairs or something like this.

iainbeeston · 2015-02-14T11:55:24Z

Right. I'll try rewriting my cop in that style and notify you when it's ready for a fresh code review.

I've added a new variant to the ruby19 hash style, that behaves the same as the regular 1.9 style, except when a hash has any keys that cannot be represented in the new style. In that case, it expects all of the keys in the hash to be hash rockets, to avoid hashes that mix the two styles in the same hash.

iainbeeston · 2015-02-15T15:31:54Z

@bbatsov I've finished rewriting my new hash-style cop now.

I've named it ruby19_no_mixed_keys. It works the same as the regular ruby19 style, but if there are keys that can't be represented in the new style (eg. non-symbols), it expects all of the keys in that hash to use hash rocket style, so we don't get hashes that mix both hash rockets and new-style hash keys.

iainbeeston · 2015-02-15T15:32:04Z

Please let me know what you think

New hash syntax cop that reports mixed syntax in the same hash

bbatsov · 2015-02-16T21:21:08Z

👍 Looks good to me. Thanks!

iainbeeston · 2015-02-17T06:51:57Z

Excellent, thanks!

iainbeeston reviewed Feb 6, 2015
View reviewed changes

iainbeeston force-pushed the dont_mix_hash_syntax branch from 5851f92 to c0a1dc9 Compare February 6, 2015 16:10

iainbeeston force-pushed the dont_mix_hash_syntax branch from c0a1dc9 to 1a7af4f Compare February 8, 2015 08:28

iainbeeston force-pushed the dont_mix_hash_syntax branch from 1a7af4f to fb1d8bf Compare February 8, 2015 08:31

iainbeeston force-pushed the dont_mix_hash_syntax branch from fb1d8bf to 11e60cd Compare February 13, 2015 13:22

bbatsov reviewed Feb 13, 2015
View reviewed changes

bbatsov mentioned this pull request Feb 14, 2015

Allow hash rockets in hashes with symbol values #1589

Merged

iainbeeston force-pushed the dont_mix_hash_syntax branch from 11e60cd to 8f19dec Compare February 15, 2015 15:26

iainbeeston force-pushed the dont_mix_hash_syntax branch from 8f19dec to 760ce1e Compare February 15, 2015 15:27

bbatsov added a commit that referenced this pull request Feb 16, 2015

Merge pull request #1641 from iainbeeston/dont_mix_hash_syntax

4fa55b4

New hash syntax cop that reports mixed syntax in the same hash

bbatsov merged commit 4fa55b4 into rubocop:master Feb 16, 2015

iainbeeston deleted the dont_mix_hash_syntax branch April 26, 2019 05:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New hash syntax cop that reports mixed syntax in the same hash #1641

New hash syntax cop that reports mixed syntax in the same hash #1641

iainbeeston commented Feb 6, 2015

iainbeeston Feb 6, 2015

iainbeeston commented Feb 6, 2015

jdickey commented Feb 6, 2015

iainbeeston commented Feb 6, 2015

jdickey commented Feb 7, 2015

iainbeeston commented Feb 7, 2015

iainbeeston commented Feb 8, 2015

coveralls commented Feb 8, 2015

coveralls commented Feb 8, 2015

coveralls commented Feb 8, 2015

iainbeeston commented Feb 13, 2015

bbatsov commented Feb 13, 2015

bbatsov Feb 13, 2015

iainbeeston Feb 13, 2015

iainbeeston commented Feb 13, 2015 via email

iainbeeston commented Feb 13, 2015 via email

bbatsov commented Feb 13, 2015

iainbeeston commented Feb 13, 2015 via email

bbatsov commented Feb 13, 2015

iainbeeston commented Feb 13, 2015 via email

iainbeeston commented Feb 13, 2015

bbatsov commented Feb 14, 2015

iainbeeston commented Feb 14, 2015 via email

bbatsov commented Feb 14, 2015

iainbeeston commented Feb 14, 2015 via email

iainbeeston commented Feb 15, 2015

iainbeeston commented Feb 15, 2015

bbatsov commented Feb 16, 2015

iainbeeston commented Feb 17, 2015 via email

New hash syntax cop that reports mixed syntax in the same hash #1641

New hash syntax cop that reports mixed syntax in the same hash #1641

Conversation

iainbeeston commented Feb 6, 2015

iainbeeston Feb 6, 2015

Choose a reason for hiding this comment

iainbeeston commented Feb 6, 2015

jdickey commented Feb 6, 2015

iainbeeston commented Feb 6, 2015

jdickey commented Feb 7, 2015

iainbeeston commented Feb 7, 2015

iainbeeston commented Feb 8, 2015

coveralls commented Feb 8, 2015

coveralls commented Feb 8, 2015

coveralls commented Feb 8, 2015

iainbeeston commented Feb 13, 2015

bbatsov commented Feb 13, 2015

bbatsov Feb 13, 2015

Choose a reason for hiding this comment

iainbeeston Feb 13, 2015

Choose a reason for hiding this comment

iainbeeston commented Feb 13, 2015 via email

iainbeeston commented Feb 13, 2015 via email

bbatsov commented Feb 13, 2015

iainbeeston commented Feb 13, 2015 via email

bbatsov commented Feb 13, 2015

iainbeeston commented Feb 13, 2015 via email

iainbeeston commented Feb 13, 2015

bbatsov commented Feb 14, 2015

iainbeeston commented Feb 14, 2015 via email

bbatsov commented Feb 14, 2015

iainbeeston commented Feb 14, 2015 via email

iainbeeston commented Feb 15, 2015

iainbeeston commented Feb 15, 2015

bbatsov commented Feb 16, 2015

iainbeeston commented Feb 17, 2015 via email