-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Ignore UnusedBlockArgument for keyword arguments #2314
Conversation
85d4b0f
to
86ae301
Compare
Apart from the failing build, the change looks reasonable to me. |
context 'when a keyword argument is not used' do | ||
let(:source) { <<-END } | ||
1.times do |foo:| | ||
puts 'foo' |
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.
Something like define_method
is the only reasonable use of keyword arguments with blocks I can think of, so it would be a better example here.
e1aeb97
to
b83fbc2
Compare
The build still fails for ruby 1.9.3 because it's unable to parse code with keyword arguments successfully. Any good ideas on how to work around that? I could return from the test early if |
@volkert you can reference the circular argument reference spec https://github.com/bbatsov/rubocop/blob/aa7d73992fcd1abbf3b57e56e6d9888a8e45d682/spec/rubocop/cop/lint/circular_argument_reference_spec.rb#L23 |
b83fbc2
to
627912c
Compare
@maxjacobson thanks! The build now passes for all versions. |
Btw, is the real issue the reporting or the suggested correction? I think it's worth reporting those by default, unless someone opts it's not (meaning this should probably be a config option). |
@volkert ping |
d83a571
to
6b3940d
Compare
expect(cop.highlights).to eq(['bar']) | ||
expect(cop.offenses.first.message).to eq( | ||
'Unused block argument - `bar`. ' \ | ||
"You can omit the argument if you don't care about it." |
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.
Sorry for the late response! I made it configurable through AllowUnusedKeywordArguments
now. I'm not sure about the error message here, though. It' actually not true that the argument can be omitted. If it is omitted ruby will complain about an unknown keyword. What do you think?
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.
If the user controls both the caller and callee, they can remove an unneeded keyword arg on both sides. If they only control the callee, then that is a problem.
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.
Well, this message didn't really take keyword args into account. Maybe it should be different for them?
@bbatsov Anything left to improve here? I'd be happy to help bringing this one to master! |
You never replied to this comment of mine. |
Actually both. The suggested correction broke the code and I'm not sure how useful it is to report unused keywords, especially if the only way of fixing it is adding an option to ignore keywords. There is no good way of improving your code to prevent the violation. But I'd be fine with how it is right now. |
@bbatsov so, how do we proceed with this? |
I'm still not sure what exactly is this change supposed to fix. Some real examples would help. |
I have a test which expects a method to be called with keyword arguments that I want to run an expect on:
(similar to my real code) If I run rubocop on this, it complains that:
The problem with the renaming is that it is a keyword argument, so the name matters here. If I replace it with an underscore or prefix it, rubocop is happy but ruby is sad because of a missing keyword.
So it's either ruby or rubocop complaining, there is no way of making both happy. My first approach for the PR to fix this was to not check for unused keywords at all because I cannot think of a situation where one is able to rename the keyword. But since you preferred having an opt-out for this, I changed my PR. This is similar to #2304. Sorry for causing so much confustion. I hope I managed to explain my situation and we can bring this to master soon ;) |
6b3940d
to
7698a4c
Compare
@bbatsov any updates on this? |
Sorry for the late response - too much things to do... I see your point now. Rebase this and I'll accept it. |
b071412
to
fc7aad3
Compare
@bbatsov I rebased it and it should be fine to merge it now. I had to do some minor changes for the tests/rubocop to pass. |
@@ -420,6 +420,7 @@ | |||
* [#2393](https://github.com/bbatsov/rubocop/pull/2393): `Style/MethodCallParentheses` doesn't fail on `obj.method ||= func()`. ([@alexdowad][]) | |||
* [#2344](https://github.com/bbatsov/rubocop/pull/2344): When autocorrecting, `Style/ParallelAssignment` reorders assignment statements, if necessary, to avoid breaking code. ([@alexdowad][]) | |||
* `Style/MultilineOperationAlignment` does not try to align the receiver and selector of a method call if both are on the LHS of an assignment. ([@alexdowad][]) | |||
* [#2314](https://github.com/bbatsov/rubocop/pull/2314): Ignore `UnusedBlockArgument` for keyword arguments. ([@volkert][]) |
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'll have to move this to the master
section.
@volkert Just address my note about the outdated position of the changelog entry. |
fc7aad3
to
96125dd
Compare
Done! |
@@ -1,6 +1,7 @@ | |||
# Change log | |||
|
|||
## master (unreleased) | |||
* [#2314](https://github.com/bbatsov/rubocop/pull/2314): Ignore `UnusedBlockArgument` for keyword arguments. ([@volkert][]) |
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 should be 3 lines down under bug fixes
I guess.
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.
fixed
96125dd
to
8f01fbe
Compare
👍 Great success! |
This PR is related to #2304 with a fix for unused block arguments.
Rubocop currently suggests to prepend an underscore to unused keyword arguments for blocks. This would break the keyword argument and theres no other way around it but ignoring the fact that there are unused keyword arguments.