-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[Fix #7737] Add new Style/UnnecessaryArguments
cop
#7761
Conversation
eece259
to
b1af511
Compare
I think that's not the best approach to building such a cop. A much better one would be to avoid hardcoding anything and build the rules for the methods and their redundant params via the configuration of the cop. |
a329f27
to
9270cab
Compare
when "''", '""' | ||
'' | ||
when '" "', "' '" | ||
' ' |
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.
This is a bad workaround & will fail for string with multiple spaces. Can anyone help me fix this?
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.
I guess first you can squeeze the extra spaces before passing them to the cases. Maybe someone else from @rubocop-hq/rubocop-core has better ideas.
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.
You could convert the configured values into AST nodes and compare directly against node.arguments.first
.
I played a bit with it and got as far as this:
require 'parser/current'
def on_send(node)
return unless redundant_argument?(node)
# ...
end
def redundant_argument?(node)
redundant_argument = redundant_arg_for_method(node.method_name.to_s)
return false if redundant_argument.nil?
node.arguments.first == redundant_argument
end
def redundant_arg_for_method(method_name)
return nil unless cop_config['RedundantArguments'].key?(method_name)
@mem ||= {}
@mem[method_name] ||= begin
arg = cop_config['RedundantArguments'].fetch(method_name)
buffer = Parser::Source::Buffer.new('(string)', 1)
buffer.source = arg.inspect
builder = RuboCop::AST::Builder.new
Parser::CurrentRuby.new(builder).parse(buffer)
end
end
I am not completely sure that a new buffer and builder instance is needed for each configuration, so the memoization may need to be moved around a bit.
3bc5864
to
78a30de
Compare
@bbatsov I have changed cop logic to be configurable for any method & argument. |
@tejasbubane Looks good to me overall. I'd only expand the docs to mention that people should be aware that the cop can't tell apart methods with the same name in different classes, so they should be careful with the cop. |
And that this is limited to methods that take a single param. To make it more generic we should be passing both a param position and a param in the config (e.g. if the redundant param is the second one), but I guess such cases are quite uncommon. |
MSG = 'Use `%<without_argument>s` instead of `%<with_argument>s`.' | ||
|
||
def on_send(node) | ||
arg = cop_config['RedundantArguments'][node.method_name.to_s] |
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.
Might be a good idea to add some validation logic for the config, so we can present the users with more meaningful error messages if they mess something up.
# "first second".split | ||
# A.foo | ||
class RedundantArguments < Cop | ||
MSG = 'Use `%<without_argument>s` instead of `%<with_argument>s`.' |
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.
I guess it'd be clearer to say that the arg is redundant.
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.
I was going to write something about us not knowing the type of the receiver, but I noticed just now that it’s been covered already in the issue #7737 (comment)
when "''", '""' | ||
'' | ||
when '" "', "' '" | ||
' ' |
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.
You could convert the configured values into AST nodes and compare directly against node.arguments.first
.
I played a bit with it and got as far as this:
require 'parser/current'
def on_send(node)
return unless redundant_argument?(node)
# ...
end
def redundant_argument?(node)
redundant_argument = redundant_arg_for_method(node.method_name.to_s)
return false if redundant_argument.nil?
node.arguments.first == redundant_argument
end
def redundant_arg_for_method(method_name)
return nil unless cop_config['RedundantArguments'].key?(method_name)
@mem ||= {}
@mem[method_name] ||= begin
arg = cop_config['RedundantArguments'].fetch(method_name)
buffer = Parser::Source::Buffer.new('(string)', 1)
buffer.source = arg.inspect
builder = RuboCop::AST::Builder.new
Parser::CurrentRuby.new(builder).parse(buffer)
end
end
I am not completely sure that a new buffer and builder instance is needed for each configuration, so the memoization may need to be moved around a bit.
@tejasbubane ping :-) |
78a30de
to
156c9bd
Compare
156c9bd
to
bd28ddf
Compare
bd28ddf
to
1b8d5d8
Compare
@bbatsov Changes done. |
Technically this cop is unsafe, as the actual default argument to join is |
I'm closing this PR due to no recent activity. Feel free to re-open it if you ever come back to it. |
@bbatsov Looks like I don't have the rights to reopen this PR. Do you mind if I open a new one? Thought I should ask first because opening a new one means losing context from the discussions here. |
@marcandre Thanks. The cop is marked unsafe. Also, these kernel variables are deprecated since 2.7 |
Can't either. Says the branch was force-pushed or recreated. Not sure why github prevents it. Anyways, you can open a new one.
Good, I must have forgotten to check, sorry. |
Closes #7737
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 RuboCop for itself, and generates the documentation.