-
-
Notifications
You must be signed in to change notification settings - Fork 260
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
Add Rails/EnumKeywordArgs cop #1239
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
* [#1239](https://github.com/rubocop/rubocop-rails/pull/1239): Add new `Rails/EnumKeywordArgs`. ([@maxprokopiev][]) | ||
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -423,6 +423,14 @@ Rails/EnumHash: | |||||||||||
Include: | ||||||||||||
- app/models/**/*.rb | ||||||||||||
|
||||||||||||
Rails/EnumKeywordArgs: | ||||||||||||
Description: 'Use positional arguments over keyword arguments when defining enums.' | ||||||||||||
StyleGuide: 'https://rails.rubystyle.guide#enums' | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you remove it because it follows a different style guide rule?
Suggested change
|
||||||||||||
Enabled: pending | ||||||||||||
VersionAdded: '<<next>>' | ||||||||||||
Comment on lines
+429
to
+430
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add the
Suggested change
|
||||||||||||
Include: | ||||||||||||
- app/models/**/*.rb | ||||||||||||
|
||||||||||||
Rails/EnumUniqueness: | ||||||||||||
Description: 'Avoid duplicate integers in hash-syntax `enum` declaration.' | ||||||||||||
Enabled: true | ||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,116 @@ | ||||||||||||
# frozen_string_literal: true | ||||||||||||
|
||||||||||||
module RuboCop | ||||||||||||
module Cop | ||||||||||||
module Rails | ||||||||||||
# Looks for enums written with keyword arguments. | ||||||||||||
# | ||||||||||||
# Defining enums with keyword arguments is deprecated | ||||||||||||
# and will be removed in Rails 7.3. | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||
# | ||||||||||||
# Positional arguments should be used instead: | ||||||||||||
# | ||||||||||||
# @example | ||||||||||||
# # bad | ||||||||||||
# enum status: { active: 0, archived: 1 }, _prefix: true | ||||||||||||
# | ||||||||||||
# # good | ||||||||||||
# enum :status, { active: 0, archived: 1 }, prefix: true | ||||||||||||
# | ||||||||||||
class EnumKeywordArgs < Base | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about the name
Suggested change
|
||||||||||||
extend AutoCorrector | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that it has been supported since Rails 5.0:
Suggested change
|
||||||||||||
|
||||||||||||
MSG_ARGS = <<~MSG.chomp | ||||||||||||
Enum defined with keyword arguments found in `%<enum>s` enum declaration. Use positional arguments instead. | ||||||||||||
MSG | ||||||||||||
MSG_OPTS = <<~MSG.chomp | ||||||||||||
Enum defined with deprecated options found in `%<enum>s` enum declaration. Use options without the `_` prefix. | ||||||||||||
MSG | ||||||||||||
|
||||||||||||
RESTRICT_ON_SEND = %i[enum].freeze | ||||||||||||
|
||||||||||||
def_node_matcher :enum?, <<~PATTERN | ||||||||||||
(send nil? :enum (hash $...)) | ||||||||||||
PATTERN | ||||||||||||
|
||||||||||||
def_node_matcher :enum_with_options?, <<~PATTERN | ||||||||||||
(send nil? :enum $_ ${array hash} $_) | ||||||||||||
PATTERN | ||||||||||||
|
||||||||||||
def_node_matcher :enum_values, <<~PATTERN | ||||||||||||
(pair $_ ${array hash}) | ||||||||||||
PATTERN | ||||||||||||
|
||||||||||||
def_node_matcher :enum_options, <<~PATTERN | ||||||||||||
(pair $_ $_) | ||||||||||||
PATTERN | ||||||||||||
|
||||||||||||
def on_send(node) | ||||||||||||
check_and_correct_keyword_args(node) | ||||||||||||
check_enum_options(node) | ||||||||||||
end | ||||||||||||
|
||||||||||||
private | ||||||||||||
|
||||||||||||
def check_and_correct_keyword_args(node) | ||||||||||||
enum?(node) do |pairs| | ||||||||||||
pairs.each do |pair| | ||||||||||||
key, values = enum_values(pair) | ||||||||||||
next unless key | ||||||||||||
|
||||||||||||
correct_keyword_args(node, key, values, pairs[1..]) | ||||||||||||
end | ||||||||||||
end | ||||||||||||
end | ||||||||||||
|
||||||||||||
def correct_keyword_args(node, key, values, options) | ||||||||||||
add_offense(values, message: format(MSG_ARGS, enum: enum_name(key))) do |corrector| | ||||||||||||
corrector.replace(node, "enum #{source(key)}, #{values.source}#{correct_options(options)}") | ||||||||||||
end | ||||||||||||
end | ||||||||||||
|
||||||||||||
def correct_options(options) | ||||||||||||
corrected_options = options.map do |pair| | ||||||||||||
name = if pair.key.source[0] == '_' | ||||||||||||
pair.key.source[1..] | ||||||||||||
else | ||||||||||||
pair.key.source | ||||||||||||
end | ||||||||||||
|
||||||||||||
"#{name}: #{pair.value.source}" | ||||||||||||
end.join(', ') | ||||||||||||
", #{corrected_options}" unless corrected_options.empty? | ||||||||||||
end | ||||||||||||
|
||||||||||||
def check_enum_options(node) | ||||||||||||
enum_with_options?(node) do |key, _, options| | ||||||||||||
options.children.each do |option| | ||||||||||||
name, = enum_options(option) | ||||||||||||
add_offense(name, message: format(MSG_OPTS, enum: enum_name(key))) if name.source[0] == '_' | ||||||||||||
end | ||||||||||||
end | ||||||||||||
end | ||||||||||||
|
||||||||||||
def enum_name(key) | ||||||||||||
case key.type | ||||||||||||
when :sym, :str | ||||||||||||
key.value | ||||||||||||
else | ||||||||||||
key.source | ||||||||||||
end | ||||||||||||
end | ||||||||||||
|
||||||||||||
def source(elem) | ||||||||||||
case elem.type | ||||||||||||
when :str | ||||||||||||
elem.value.dump | ||||||||||||
when :sym | ||||||||||||
elem.value.inspect | ||||||||||||
else | ||||||||||||
elem.source | ||||||||||||
end | ||||||||||||
end | ||||||||||||
end | ||||||||||||
end | ||||||||||||
end | ||||||||||||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
# frozen_string_literal: true | ||
|
||
RSpec.describe RuboCop::Cop::Rails::EnumKeywordArgs, :config do | ||
context 'when keyword arguments are used' do | ||
context 'with %i[] syntax' do | ||
it 'registers an offense' do | ||
expect_offense(<<~RUBY) | ||
enum status: { active: 0, archived: 1 }, _prefix: true | ||
^^^^^^^^^^^^^^^^^^^^^^^^^^ Enum defined with keyword arguments found in `status` enum declaration. Use positional arguments instead. | ||
RUBY | ||
end | ||
end | ||
|
||
context 'with options prefixed with `_`' do | ||
it 'registers an offense' do | ||
expect_offense(<<~RUBY) | ||
enum :status, { active: 0, archived: 1 }, _prefix: true, _suffix: true | ||
^^^^^^^ Enum defined with deprecated options found in `status` enum declaration. Use options without the `_` prefix. | ||
^^^^^^^ Enum defined with deprecated options found in `status` enum declaration. Use options without the `_` prefix. | ||
RUBY | ||
end | ||
end | ||
|
||
context 'when the enum name is a string' do | ||
it 'registers an offense' do | ||
expect_offense(<<~RUBY) | ||
enum "status" => { active: 0, archived: 1 } | ||
^^^^^^^^^^^^^^^^^^^^^^^^^^ Enum defined with keyword arguments found in `status` enum declaration. Use positional arguments instead. | ||
RUBY | ||
end | ||
end | ||
|
||
context 'when the enum name is not a literal' do | ||
it 'registers an offense' do | ||
expect_offense(<<~RUBY) | ||
enum KEY => { active: 0, archived: 1 } | ||
^^^^^^^^^^^^^^^^^^^^^^^^^^ Enum defined with keyword arguments found in `KEY` enum declaration. Use positional arguments instead. | ||
RUBY | ||
end | ||
end | ||
|
||
it 'autocorrects' do | ||
expect_offense(<<~RUBY) | ||
enum status: { active: 0, archived: 1 } | ||
^^^^^^^^^^^^^^^^^^^^^^^^^^ Enum defined with keyword arguments found in `status` enum declaration. Use positional arguments instead. | ||
RUBY | ||
|
||
expect_correction(<<~RUBY) | ||
enum :status, { active: 0, archived: 1 } | ||
RUBY | ||
end | ||
|
||
it 'autocorrects options too' do | ||
expect_offense(<<~RUBY) | ||
enum status: { active: 0, archived: 1 }, _prefix: true, _suffix: true | ||
^^^^^^^^^^^^^^^^^^^^^^^^^^ Enum defined with keyword arguments found in `status` enum declaration. Use positional arguments instead. | ||
RUBY | ||
|
||
expect_correction(<<~RUBY) | ||
enum :status, { active: 0, archived: 1 }, prefix: true, suffix: true | ||
RUBY | ||
end | ||
end | ||
|
||
context 'when positional arguments are used' do | ||
it 'does not register an offense' do | ||
expect_no_offenses('enum :status, { active: 0, archived: 1 }, prefix: true') | ||
end | ||
end | ||
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.