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
Include expanded EnforcedStyle options when --no-auto-gen-enforced-style is given #12388
Include expanded EnforcedStyle options when --no-auto-gen-enforced-style is given #12388
Conversation
83e7056
to
9613eba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! 👍
rejected_keys << /^EnforcedStyle\w*/ unless auto_gen_enforced_style? | ||
cfg.reject { |key| include_or_match?(rejected_keys, key) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a logic by which the following would work, instead of creating the new include_or_match
method?
rejected_keys << /^EnforcedStyle\w*/ unless auto_gen_enforced_style? | |
cfg.reject { |key| include_or_match?(rejected_keys, key) } | |
rejected_keys << 'EnforcedStyle' unless auto_gen_enforced_style? | |
cfg.reject do |key| | |
rejected_keys.any? { |rejected_key| key.start_with?(rejected_key) } | |
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this would work too! However then it only works for prefixed strings of course. I tried to anticipate for possible use cases where a key would not necessarily just start with the rejected key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if the current logic is left as is, /^EnforcedStyle\w*/
appears like it should be /\AEnforcedStyle\w*/
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but any regexp could be used. :)
Edit: Ah, sorry, understood you wrong. You're right, \A should be added! What is your opinion on using regexes?
I'm fine with the proposed change. Thanks! |
It seems to me that when the
--no-auto-gen-enforced-style
is given, not onlyEnforcedStyle
should be rejected, but also all 'expanded'EnforcedStyle
options as well, like for exampleEnforcedStyleForEmptyBraces
. In order to achieve this I used a Regex when checking which keys should be rejected.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}.md
if the new code introduces user-observable changes. See changelog entry format for details.