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

default key in Style/PercentLiteralDelimiters #4038

Merged
merged 4 commits into from
Mar 3, 2017
Merged

default key in Style/PercentLiteralDelimiters #4038

merged 4 commits into from
Mar 3, 2017

Conversation

kddnewton
Copy link
Contributor

Oftentimes people want to set the default percent literal delimiter to [], but have to override every available option in order to get that behavior. This commit adds the ability to specify an 'all' key that will specify the default for every possible delimiter, which you can then overwrite with others.

@bbatsov
Copy link
Collaborator

bbatsov commented Mar 2, 2017

I think default would be a better name than all. It should be explained properly that you can still override the default for all delimiter by explicitly setting some value for a particular delimiter.

@kddnewton kddnewton changed the title all key in Style/PercentLiteralDelimiters default key in Style/PercentLiteralDelimiters Mar 2, 2017
@kddnewton
Copy link
Contributor Author

@bbatsov Yeah I agree. I've updated the code and merged in master. Do you have further thoughts on this?

@@ -4,6 +4,9 @@ module RuboCop
module Cop
module Style
# This cop enforces the consistent usage of `%`-literal delimiters.
# Specify the 'default' key to set all preferred delimiters at once. You
# can continue to specify individual preferred delimiters to override the
# default.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Btw, you can give a few code examples here.

def preferred_delimiters
@preferred_delimiters ||=
if cop_config['PreferredDelimiters'].key?('default')
Hash[%w(% %i %I %q %Q %r %s %w %W %x).map do |type|
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd make this array a constant.

Copy link
Collaborator

Choose a reason for hiding this comment

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

And we can probably add some error checking for unknown delimiters in case users mistype something.

@@ -912,16 +912,8 @@ Style/ParenthesesAroundCondition:

Style/PercentLiteralDelimiters:
PreferredDelimiters:
'%': ()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Although this is more or less clear you can add some comment here explaining the configuration of the cop.

@bbatsov
Copy link
Collaborator

bbatsov commented Mar 3, 2017

Apart from the failing and my small remarks, the changes look ok to me.

@kddnewton
Copy link
Contributor Author

Great, I've updated with that feedback in mind. Let me know what you think. Thanks for the quick responses!

@bbatsov
Copy link
Collaborator

bbatsov commented Mar 3, 2017

Looks good! You'll just have to rebase on top of the current master.

Oftentimes people want to set the default percent literal delimiter to [], but have to override every available option in order to get that behavior. This commit adds the ability to specify an 'all' key that will specify the default for every possible delimiter, which you can then overwrite with others.
Create PERCENT_LITERAL_TYPES constant for the different supported types and ensure valid configuration is passed to the PercentLiteralDelimiters cop.
@kddnewton
Copy link
Contributor Author

Sounds good, just rebased.

@bbatsov bbatsov merged commit 7a84fc8 into rubocop:master Mar 3, 2017
@kddnewton kddnewton deleted the all-in-percent-literal-delimiters branch March 3, 2017 19:07
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