[Fix #1343] False negatives for Rails/EnumSyntax
#1348
Conversation
lib/rubocop/cop/rails/enum_syntax.rb
Outdated
| @@ -107,6 +96,10 @@ def enum_name(elem) | |||
| end | |||
| end | |||
|
|
|||
| def option_key?(pair) | |||
| pair.key.source[0] == '_' | |||
There was a problem hiding this comment.
Hmm, wouldn't this approach result in false negatives for cases like enum _key, {val1: 0, val2: 1}?
It might be more appropriate to exclude only the key names that are meaningful as options, such as _prefix, _suffix, _scopes, _default, and _instance_methods. What do you think?
There was a problem hiding this comment.
I guess that's right, yeah.
I wonder if anyone would actually have a db schema where this would matter but it's easy to support and rails doesn't have a problem with it.
Earlopain
force-pushed
the
enum-syntax-non-literal
branch
from
99eb09b
to
730dbf4
Compare
lib/rubocop/cop/rails/enum_syntax.rb
Outdated
| @@ -25,6 +25,7 @@ class EnumSyntax < Base | |||
| MSG_OPTIONS = 'Enum defined with deprecated options in `%<enum>s` enum declaration. Remove the `_` prefix.' | |||
| RESTRICT_ON_SEND = %i[enum].freeze | |||
| OPTION_NAMES = %w[prefix suffix scopes default].freeze | |||
There was a problem hiding this comment.
The following could be added at this point :-)
| OPTION_NAMES = %w[prefix suffix scopes default].freeze | |
| OPTION_NAMES = %w[prefix suffix scopes default instance_methods].freeze |
There was a problem hiding this comment.
Yeah. Looks like I somehow linked an older commit in the other PR.
I've added it. There is now also validate but that only works with the new syntax and shouldn't be considered here.
Earlopain
force-pushed
the
enum-syntax-non-literal
branch
from
730dbf4
to
0907e9f
Compare
lib/rubocop/cop/rails/enum_syntax.rb
Outdated
| @@ -24,7 +24,10 @@ class EnumSyntax < Base | |||
| MSG = 'Enum defined with keyword arguments in `%<enum>s` enum declaration. Use positional arguments instead.' | |||
| MSG_OPTIONS = 'Enum defined with deprecated options in `%<enum>s` enum declaration. Remove the `_` prefix.' | |||
| RESTRICT_ON_SEND = %i[enum].freeze | |||
| OPTION_NAMES = %w[prefix suffix scopes default].freeze | |||
|
|
|||
| # From https://github.com/rails/rails/blob/5c0b7496ab32c25c80f6d1bdc8b32ec6f75ce1e4/activerecord/lib/active_record/enum.rb#L222 | |||
There was a problem hiding this comment.
I like this comment.
Can you tweak to the following? This clarifies the reference version.
| # From https://github.com/rails/rails/blob/5c0b7496ab32c25c80f6d1bdc8b32ec6f75ce1e4/activerecord/lib/active_record/enum.rb#L222 | |
| # From https://github.com/rails/rails/blob/v7.2.1/activerecord/lib/active_record/enum.rb#L231 |
There was a problem hiding this comment.
I'll do tag references for these in the future, that's much better 👍
Earlopain
force-pushed
the
enum-syntax-non-literal
branch
from
0907e9f
to
4a878ba
Compare
|
I apologize for not explaining myself clearly. If you are going to address Or, If it seems better to separate the commits due to the user impact, it might also make sense to have a separate changelog entry, so it would be better to open it as a separate PR. Could you handle it in the way you prefer? |
I don't believe there is any issue with just looking at all value types. It just translates `foo: bar` into `:foo, bar`. Additionally, add `instance_methods` as a rails option. Not doing this would result autocorrect treating it as a enum column
Earlopain
force-pushed
the
enum-syntax-non-literal
branch
from
4a878ba
to
dbdee93
Compare
|
I squashed, seems fine to just do here. This only really matters with the previous change anyways, since it now specifically looks at what the option key looks like. |
Earlopain
changed the title
Rails/EnumSyntax
|
Thanks! |
Fix #1343. I don't believe there is any issue with just looking at all value types. It just translates
foo: barinto:foo, bar.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.