Allow asking to avoid mixing hash shorthand styles in single hashes #10776

h-lame · 2022-06-29T19:07:46Z

For those who want to use hash shorthand style, but don't want to mix shorthand and explicit styles in the same hash we provide a new option for EnforcedShorthandSyntax called consistent that raises an offence if we've mixed styles.

E.g.

# bad
{foo: , bar: bar}

# good
{foo:, bar:}

# bad
{foo: , bar: baz}

# good
{foo: foo, bar: baz}

Requesting help 🙋

I don't particularly like the implementation. Prior to this commit HashSyntax mixed in HashShorthandSyntax but was otherwise unaware of it. With this change I've had to make HashSyntax aware of HashShorthandSyntax so it can call on_hash_for_mixed_shorthand in on_hash. I don't think there's a way to enforce a no mixing style at the pair level meaning we can't do this in on_pair already present in HashShorthandSyntax and have to use on_hash to look at the whole hash in one go. Would love some advice on this.

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). (n/a)
Feature branch is up-to-date with master (if not - rebase it). (current rebase against - 0984886)
Squashed related commits together. (n/a - single commit)
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.

jonas054

I don't have any problems with the dependency from HashSyntax to the mixin. It all looks good to me. But I'm missing the new supported style in default.yml.

lib/rubocop/cop/mixin/hash_shorthand_syntax.rb

h-lame · 2022-07-14T20:35:41Z

I don't have any problems with the dependency from HashSyntax to the mixin.

Thanks for looking - I'll leave it as-is then!

But I'm missing the new supported style in default.yml.

My bad, I didn't realise I needed to update that too - all done now though! Rebased again too.

bbatsov · 2022-08-04T12:46:08Z

I'm OK with the proposal, but I don't like the name of the config option. I'm think of something like (keep)_consistent, but I'm open to other suggestions. Naming is hard. :-)

dvandersluis · 2022-08-04T12:55:56Z

We use consistent as a style name elsewhere so it’d be consistent 😂

h-lame · 2022-08-04T19:03:14Z

I admit I'm not sold on prefer_short but I thought the no_mixing gave some consistency with the no_mixed_keys and ruby19_no_mixed_keys values for SupportedStyles. Admittedly, no_mixed_keys is clear what it's not mixing, I'm not sure what we call this shorthand syntax - it's not keys, and values doesn't feel right to me. Some alternates:

prefer_short_no_mixing - what I came up with originally
always_no_mixing - clear that it's like the value always in intent, but has a suffix to suggest a difference (things won't be mixed)
always_keep_consistent - again, clear that it's like always and uses a suffix to suggest a difference (however, it's less clear that there's an implied punctuation between always and keep_consistent (same problem if we drop keep here - always_consistent doesn't work either)
always_no_mixed_omissions - is "omissions" the right term here for what we're not allowing to be mixed? Doesn't really feel like it
for_entire_hashes_only - going away from all the other options, this feels like it's clearer, but far from perfect

I'll keep pondering it 🤔

bbatsov · 2022-08-06T05:53:23Z

Just keep in mind that's not really a supported style, but rather the value of EnforcedShorthandSyntax, so the expectation here therefore my suggestion for consistent, as everything but "never" implies some usage of the shorthand syntax anyways. I don't like always_something as it's not really always then.

h-lame · 2022-08-07T21:20:24Z

I don't really have a better suggestion than consistent. What I think it lacks is a hint that it's going to prefer the shorthand syntax and re-write your hashes to use it if it can. I don't think consistent is clear enough. But, as I said, I don't have a better suggestion, and, as you say, it's kinda implied by the name of the option anyway: EnforcedShorthandSyntax. The docs can explain it too. I have rebased and updated to use consistent as the option value.

dvandersluis · 2022-08-08T18:35:54Z

spec/rubocop/cop/style/hash_syntax_spec.rb

+      it 'registers an offense when some hash values are omitted but they cannot all be omitted' do
+        expect_offense(<<~RUBY)
+          {foo:, bar: baz}
+           ^^^ Do not mix explicit and implicit hash values.  Explicit the hash value.


Explicit the hash value doesn't really make sense (I see now that it's used on the old messages too but I still don't think it makes sense).

Suggested change

^^^ Do not mix explicit and implicit hash values. Explicit the hash value.

^^^ Do not mix explicit and implicit hash values. Include the hash value.

Also there's an extra space after the period.

I agree that it doesn't make much sense, but I was reusing the existing error message for the never option. Should I change that one too - are changes to existing error messages a breaking change?

We can agree to disagree on the period double space formatting though 😉. I will remove it though as it does seem more consistent with the rest of the error messages.

We could change it as a separate PR if you'd prefer, but if you're willing, change it everywhere please! Changing messages aren't considered breaking.

Added a commit to reword the existing message before adding the new feature.

It was "explicit the hash value", which on reflection, doesn't _quite_ make sense so we've changed it to "include the hash value", which does.

For those who want to use hash shorthand style, but don't want to mix shorthand and explicit styles in the same hash we provide a new option for `EnforcedShorthandSyntax` called `consistent` that raises an offence if we've mixed styles. This cop prefers the shorthand syntax only if the whole hash can be written that way, but if it can't it will prefer fully explicit values. E.g. # bad {foo: , bar: bar} # good {foo:, bar:} # bad {foo: , bar: baz} # good {foo: foo, bar: baz}

dvandersluis · 2022-08-09T22:54:17Z

Thanks!

koic · 2022-08-10T00:52:40Z

@dvandersluis From next time onwards, please wait a few days after a minor version release for new features to merge. If there is an urgent bug, it will be a bugfix release. For example, I will wait for @bbatsov to merge new features before merging PR like this. For future reference. Anyway thank you for your activities :-)

`EnforcedShorthandSyntax: always` is the default setting in Rubocop even though Bozhidar himself [doesn't like the option](https://batsov.com/articles/2022/01/20/bad-ruby-hash-value-omission/). I suggest to change the default option to `consistent` because I can see the advantage of the shorthand syntax when passing on options to deeper levels. But in any other context I found myself to be confused by it. This change allows hashes to be in shorthand syntax only if the whole hash uses it consistently. See rubocop/rubocop#10776.

schmijos · 2022-09-26T13:06:06Z

@h-lame Did you make your thoughts about hashes as method arguments as well? Is something like this consistent or not?

FactoryBot.create(:customer_promotion, :claimed, customer:, store:)

h-lame · 2022-09-26T14:12:59Z

@schmijos - I haven't explicitly tested that example, but it should be consistent and not flag for change because the implementation only looks at hashes, and in the "hash" part of that method call (customer: , store:) they're already consistent. If you wanted it to do something else then I think you'd need to add a different implementation around argument lists.

* Enforce consistent hash syntax `EnforcedShorthandSyntax: always` is the default setting in Rubocop even though Bozhidar himself [doesn't like the option](https://batsov.com/articles/2022/01/20/bad-ruby-hash-value-omission/). I suggest to change the default option to `consistent` because I can see the advantage of the shorthand syntax when passing on options to deeper levels. But in any other context I found myself to be confused by it. This change allows hashes to be in shorthand syntax only if the whole hash uses it consistently. See rubocop/rubocop#10776. * Switch to either Co-authored-by: Alessandro Rodi <alessandro.rodi@renuo.ch>

h-lame force-pushed the support-no-mixing-for-3.1-hash-shorthand-syntax branch 2 times, most recently from 48dbeac to 8f07125 Compare July 5, 2022 14:32

jonas054 requested changes Jul 13, 2022

View reviewed changes

lib/rubocop/cop/mixin/hash_shorthand_syntax.rb Outdated Show resolved Hide resolved

h-lame force-pushed the support-no-mixing-for-3.1-hash-shorthand-syntax branch from 8f07125 to 21a4aa7 Compare July 14, 2022 20:33

h-lame force-pushed the support-no-mixing-for-3.1-hash-shorthand-syntax branch from 21a4aa7 to 68a5d91 Compare July 14, 2022 20:42

jonas054 approved these changes Jul 15, 2022

View reviewed changes

h-lame force-pushed the support-no-mixing-for-3.1-hash-shorthand-syntax branch from 68a5d91 to bc6a236 Compare August 7, 2022 21:20

dvandersluis reviewed Aug 8, 2022

View reviewed changes

h-lame force-pushed the support-no-mixing-for-3.1-hash-shorthand-syntax branch from bc6a236 to bdc4689 Compare August 8, 2022 20:26

h-lame added 2 commits August 9, 2022 22:08

Reword error message for requiring explicit hash values

c34b4f6

It was "explicit the hash value", which on reflection, doesn't _quite_ make sense so we've changed it to "include the hash value", which does.

h-lame force-pushed the support-no-mixing-for-3.1-hash-shorthand-syntax branch from bdc4689 to edb5399 Compare August 9, 2022 21:08

dvandersluis merged commit f53c644 into rubocop:master Aug 9, 2022

h-lame deleted the support-no-mixing-for-3.1-hash-shorthand-syntax branch August 10, 2022 07:23

jdelStrother mentioned this pull request Aug 17, 2022

"EnforcedShorthandSyntax: consistent" doesn't work? #10933

Closed

schmijos mentioned this pull request Sep 26, 2022

Enforce ~consistent~ either hash syntax renuo/applications-setup-guide#309

Merged

jandudulski mentioned this pull request Oct 6, 2022

Unexpected changes with named arguments in 1.6.0 from Style/HashSyntax standardrb/standard#375

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow asking to avoid mixing hash shorthand styles in single hashes #10776

Allow asking to avoid mixing hash shorthand styles in single hashes #10776

h-lame commented Jun 29, 2022 •

edited

jonas054 left a comment

h-lame commented Jul 14, 2022

bbatsov commented Aug 4, 2022

dvandersluis commented Aug 4, 2022

h-lame commented Aug 4, 2022 •

edited

bbatsov commented Aug 6, 2022

h-lame commented Aug 7, 2022 •

edited

dvandersluis Aug 8, 2022 •

edited

h-lame Aug 8, 2022

dvandersluis Aug 8, 2022

h-lame Aug 9, 2022

dvandersluis commented Aug 9, 2022

koic commented Aug 10, 2022

schmijos commented Sep 26, 2022 •

edited

h-lame commented Sep 26, 2022

	^^^ Do not mix explicit and implicit hash values. Explicit the hash value.
	^^^ Do not mix explicit and implicit hash values. Include the hash value.

Allow asking to avoid mixing hash shorthand styles in single hashes #10776

Allow asking to avoid mixing hash shorthand styles in single hashes #10776

Conversation

h-lame commented Jun 29, 2022 • edited

Requesting help 🙋

jonas054 left a comment

Choose a reason for hiding this comment

h-lame commented Jul 14, 2022

bbatsov commented Aug 4, 2022

dvandersluis commented Aug 4, 2022

h-lame commented Aug 4, 2022 • edited

bbatsov commented Aug 6, 2022

h-lame commented Aug 7, 2022 • edited

dvandersluis Aug 8, 2022 • edited

Choose a reason for hiding this comment

h-lame Aug 8, 2022

Choose a reason for hiding this comment

dvandersluis Aug 8, 2022

Choose a reason for hiding this comment

h-lame Aug 9, 2022

Choose a reason for hiding this comment

dvandersluis commented Aug 9, 2022

koic commented Aug 10, 2022

schmijos commented Sep 26, 2022 • edited

h-lame commented Sep 26, 2022

h-lame commented Jun 29, 2022 •

edited

h-lame commented Aug 4, 2022 •

edited

h-lame commented Aug 7, 2022 •

edited

dvandersluis Aug 8, 2022 •

edited

schmijos commented Sep 26, 2022 •

edited