[Fix #11802] Improve handling of [] and () with percent symbol arrays #12037
Conversation
| @@ -8,7 +8,7 @@ class UndefinedConfig < Base | |||
| ALLOWED_CONFIGURATIONS = %w[ | |||
| Safe SafeAutoCorrect AutoCorrect Severity StyleGuide Details Reference Include Exclude | |||
| ].freeze | |||
| RESTRICT_ON_SEND = %i[[] fetch].freeze | |||
| RESTRICT_ON_SEND = [:[], :fetch].freeze | |||
There was a problem hiding this comment.
If bracket do not contain spaces, they should not be changed.
So, only the following case that need escaping can be changed from percent literal style to bracket style:
%i[[ ]] #=> [:"[", :"]"]This case where brackets do not contain spaces should not be changed:
%i[[]] #=> [:[]]That is, it must be checked for spaces in brackets. Can you update the entire changes in this PR?
There was a problem hiding this comment.
@koic I think that makes sense to me!
But one thing that I notice is that while %i[[] one] does not currently raise an offense, rubocop still autocorrects [:[], :one] to %i[\[\] one], which feels inconsistent 🤔 Any thoughts?
There was a problem hiding this comment.
When EnforcedStyle: percent, %i[[] one] (no spaces in the brackets) will need to adjust the implementation if necessary to be accepted.
jasondoc3
force-pushed
the
consider_brackets_as_bad_for_style_symbol_array
branch
from
f9fc6e5
to
1920852
Compare
| # %i[foo\ bar baz\ quux] | ||
| # | ||
| # # bad (contains [] with spaces) | ||
| # %i[foo\ \[ \]] |
There was a problem hiding this comment.
I'd remove the escaped space after foo to make this example clearer.
| # %i[foo\ \[ \]] | ||
| # | ||
| # # bad (contains () with spaces) | ||
| # %i(foo\ \( \)) |
There was a problem hiding this comment.
I'd remove the escaped space after foo to make this example clearer.
|
@jasondoc3 ping :-) |
jasondoc3
force-pushed
the
consider_brackets_as_bad_for_style_symbol_array
branch
from
31b5b8b
to
636cff8
Compare
| @@ -57,13 +66,24 @@ def on_array(node) | |||
|
|
|||
| private | |||
|
|
|||
| def symbols_contain_spaces?(node) | |||
| def complex_content?(node) | |||
| delimiters = ['[', ']', '(', ')'] | |||
There was a problem hiding this comment.
@jasondoc3 It could be a constant instead of a variable :-)
jasondoc3
force-pushed
the
consider_brackets_as_bad_for_style_symbol_array
branch
from
636cff8
to
80acfbd
Compare
|
Thanks! |
Addresses #11802, but also accounts for
()Before submitting the PR make sure the following are checked:
[Fix #issue-number](if the related issue exists).master(if not - rebase it).bundle exec rake default. It executes all tests and runs RuboCop on its own code.{change_type}_{change_description}.mdif the new code introduces user-observable changes. See changelog entry format for details.