Skip to content

Enhance errors.add linter to check symbol as second argument #9569

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.

Sign up for GitHub

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

Merged
aduth merged 2 commits into from
Nov 9, 2023
+44 −12
Merged

Enhance errors.add linter to check symbol as second argument #9569

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b2dc44a
Enhance errors.add linter to check type argument
aduth Nov 9, 2023
b79e6b7
Fix typo argument
aduth Nov 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 6 additions & 9 deletions lib/linters/errors_add_linter.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,12 @@ class ErrorsAddLinter < RuboCop::Cop::Cop
PATTERN

def on_send(node)
unless node.arguments.last.type == :hash || errors_add_match?(node).nil?
return add_offense(node, location: :expression)
end
errors_add_match?(node) do |arguments|
type_node = arguments.last.pairs.find do |pair|
pair.key.sym_type? && pair.key.source == 'type'
end
add_offense(node, location: :expression) unless type_node
end
return unless errors_add_match?(node)
_attr, type, options = node.arguments
return if type && type.type == :sym
options = type if type && type.type == :hash
return if options && options.type == :hash && options.keys.map(&:value).include?(:type)
Copy link
Copy Markdown
Contributor Author

@aduth aduth Nov 9, 2023

Choose a reason for hiding this comment

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

My micro-optimizing brain didn't like this because it's wasteful to produce a mapped result for the entire hash when we're only interested to iterate up to the point we find the relevant key, but I couldn't fit that solution into a single line 😄

Suggested change
return if options && options.type == :hash && options.keys.map(&:value).include?(:type)
return if options && options.type == :hash && options.keys.any? { |key| key.value == :type }
Suggested change
return if options && options.type == :hash && options.keys.map(&:value).include?(:type)
return if options && options.type == :hash && options.keys.lazy.map(&:value).include?(:type)

add_offense(node, location: :expression)
end
end
end
Expand Down
41 changes: 38 additions & 3 deletions spec/lib/linters/errors_add_linter_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,22 @@
let(:config) { RuboCop::Config.new }
let(:cop) { RuboCop::Cop::IdentityIdp::ErrorsAddLinter.new(config) }

it 'registers an offense when neither type nor options are specified' do
expect_offense(<<~RUBY)
class MyModel
def my_method
errors.add(:number)
^^^^^^^^^^^^^^^^^^^ IdentityIdp/ErrorsAddLinter: Please set a unique key for this error
end
end
RUBY
end

it 'registers an offense when no options are passed' do
expect_offense(<<~RUBY)
class MyModel
def my_method
errors.add(:number, "is negative")
errors.add(:number, 'is negative')
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ IdentityIdp/ErrorsAddLinter: Please set a unique key for this error
end
end
Expand All @@ -24,7 +35,7 @@ def my_method
expect_offense(<<~RUBY)
class MyModel
def my_method
errors.add(:number, "is negative", foo: :bar)
errors.add(:number, 'is negative', foo: :bar)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ IdentityIdp/ErrorsAddLinter: Please set a unique key for this error
end
end
Expand All @@ -48,7 +59,31 @@ def validate
class MyModel
def validate
if number.negative?
errors.add(:number, "is negative", type: :is_negative)
errors.add(:number, 'is negative', type: :is_negative)
end
end
end
RUBY
end

it 'registers no offense when defining hash as second argument including "type"' do
expect_no_offenses(<<~RUBY)
class MyModel
def validate
if number.negative?
errors.add(:number, message: 'is negative', type: :is_negative)
end
end
end
RUBY
end

it 'registers no offense when type symbool is defined as second argument' do
expect_no_offenses(<<~RUBY)
class MyModel
def validate
if number.negative?
errors.add(:number, :is_negative, message: 'is negative')
end
end
end
Expand Down