Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignore symbols ending with ?/! in Style/HashSyntax #3149

Conversation

owst
Copy link
Contributor

@owst owst commented May 19, 2016

Perhaps a slightly personal taste, but I think

{ :development? => true }

reads better/is clearer than

{ development?: true }

and therefore Rubocop shouldn't suggest changing the former to the latter. (Similarly for symbols ending in ! e.g. { :raise_on_invalid! => true }.

As an example, the original line I wrote that triggered this was:

expect(Rails).to receive(:env).and_return(double(:production? => true)) 

I'd be interested to know what others think of this one.


Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • Used the same coding conventions as the rest of the project.
  • 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.
  • All tests are passing.
  • The new code doesn't generate RuboCop offenses.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.

@owst owst force-pushed the ignore_symbols_ending_with_exclamation_or_question_marks_for_hash_syntax branch 2 times, most recently from 8d6a563 to 68b4e10 Compare May 20, 2016 09:04
@bbatsov
Copy link
Collaborator

bbatsov commented May 20, 2016

As this is subjective it should be controlled via some config option.

@owst owst force-pushed the ignore_symbols_ending_with_exclamation_or_question_marks_for_hash_syntax branch 2 times, most recently from a80c1ff to 8ef1c40 Compare May 21, 2016 11:00
@owst
Copy link
Contributor Author

owst commented May 21, 2016

@bbatsov good point - done, and rebased

@owst owst force-pushed the ignore_symbols_ending_with_exclamation_or_question_marks_for_hash_syntax branch from 8ef1c40 to 9a963d8 Compare May 23, 2016 17:22
UseHashRocketsWithSymbolValues: false
# Do not suggest { a?: 1 } over { :a? => 1 } in ruby19 style
AllowHashRocketsWithSymbolValuesEndingInNonAlnum: true
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a mouthful. :-) Probably we can shorten it a bit.

I'm also not sure we should set it to true by default.

Copy link
Contributor Author

@owst owst May 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haha, yeah definitely, I struggled with it! What about

  1. AllowNonAlnumEndingSymbolHashRockets
  2. AllowHashRocketsForPunctuationEndingSymbols
  3. HashRocketsAllowedWithEndingPunctuation
  4. PreferHashRocketsForNonAlnumEndingSymbols

contrasting with the current

  • AllowHashRocketsWithSymbolValuesEndingInNonAlnum

Ok, I'll rebase and make it false by default and for now use 4.

@owst owst force-pushed the ignore_symbols_ending_with_exclamation_or_question_marks_for_hash_syntax branch 3 times, most recently from 80ab698 to 1a55467 Compare May 26, 2016 23:36
@owst
Copy link
Contributor Author

owst commented May 26, 2016

I'll repaste my reply to @bbatsov since it was hidden by my change:

haha, yeah definitely, I struggled with it! What about

  1. AllowNonAlnumEndingSymbolHashRockets
  2. AllowHashRocketsForPunctuationEndingSymbols
  3. HashRocketsAllowedWithEndingPunctuation
  4. PreferHashRocketsForNonAlnumEndingSymbols

contrasting with the current

  • AllowHashRocketsWithSymbolValuesEndingInNonAlnum

Ok, I'll rebase and make it false by default and for now use 4.

@@ -19,6 +19,10 @@
* [#3140](https://github.com/bbatsov/rubocop/pull/3140): `Style/FrozenStringLiteralComment` works with file doesn't have any tokens. ([@pocke][])
* [#3154](https://github.com/bbatsov/rubocop/issues/3154): Fix handling of `()` in `Style/RedundantParentheses`. ([@lumeet][])

### Changes

* [#3149](https://github.com/bbatsov/rubocop/pull/3149): Do not report symbols ending with ? or ! when using hash rocket syntax in `Style/HashSyntax`. ([@owst][])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changelog entry is a bit misleading after the last round of changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point @bbatsov - done

@owst owst force-pushed the ignore_symbols_ending_with_exclamation_or_question_marks_for_hash_syntax branch from 1a55467 to ec350db Compare May 27, 2016 08:05
@bbatsov bbatsov merged commit 2ae9941 into rubocop:master May 27, 2016
Neodelf pushed a commit to Neodelf/rubocop that referenced this pull request Oct 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants