-
-
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
Add new Lint/RedundantWithObject
cop
#4796
Add new Lint/RedundantWithObject
cop
#4796
Conversation
new_source = autocorrect_source('ary.each.with_object([]) { |v| v }') | ||
|
||
expect(new_source).to eq 'ary.each { |v| v }' | ||
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.
Could you add tests for without parens and/or do-end keyword block.
For example:
a.each_with_object([]) do |v|
end
a.each_with_object [] do |v|
end
a.each_with_object([]) {|v| v}
a.each_with_object [] {|v| v}
And the auto-correction for the first case generates a broken code.
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.
Oh, I didn't realize that. I'm going to look in detail later. Thanks for your advise!
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 added some test cases and fixed it with 95b0c46.
157cd0f
to
581bcf0
Compare
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.
Aside from the questions about consistency with the with_index
cop, this looks good to me.
The commits will need to be squashed.
.freeze | ||
MSG_WITH_OBJECT = 'Remove redundant `with_object`.'.freeze | ||
|
||
def_node_matcher :redundant_with_object?, <<-PATTERN |
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 expecting this pattern to be the same as the one of with_index
with index
swapped for object
, but this pattern is slightly different. The pattern for with_object
has an extra (_)
after with_object
and before args
. Is it needed for this pattern? Should it be added to the with_index
pattern?
def_node_matcher :redundant_with_index?, <<-PATTERN
(block
$(send
_ {:each_with_index :with_index})
(args
(arg _))
...)
PATTERN
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.
Yes. As you pointed out, the with_index
pattern of Lint/RedundantWithObject
cop can be improved.
def with_object_range(send) | ||
range_between( | ||
send.loc.selector.begin_pos, | ||
send.source.length |
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.
The end range is different than the with_index
cop. I saw that this was changed from send.loc.selector.end_pos + 1
when tests were added for the method call without parentheses. Does this need to be changed in the with_index
cop?
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.
Current Lint/RedundantWithObject
cop does not check the argument with_index. This is an oversight of me, in fact it would be better to handle the case with the with_index
argument. It can be improved as an implementation of Lint/RedundantWithObject
cop.
e6479e0
to
908a422
Compare
@koic Can you do the improvements suggested by @rrosenblum? |
@bbatsov Thanks for the mention. I'm considering improving |
Got it. |
Any idea why the build is currently failing on JRuby? |
I was not aware of the failing on JRuby. Since failure in master branch has been fixed, rebasing in the latest master branch may fix the failing on JRuby. If not, investigation is necessary. In that case, I'd like to investigate. |
@bbatsov May I |
Of course. |
Thanks. I'll try. |
0f2d5f1
to
b168b91
Compare
CI has failed, so I will investigate. Please wait for a little while. |
b168b91
to
343e83b
Compare
```console % bundle exec rubocop -a lib/rubocop/cop/lint/redundant_with_object.rb Inspecting 1 file C Offenses: lib/rubocop/cop/lint/redundant_with_object.rb:45:31: C: [Corrected] Use of positional arguments on #add_offense is deprecated. add_offense(node, with_object_range(send)) ^^^^^^^^^^^^^^^^^^^^^^^ 1 file inspected, 1 offense detected, 1 offense corrected ```
343e83b
to
0d1f22b
Compare
CI passed. What I did was rebase with the latest master branch, and I autocorrected it. 0d1f22b |
Follow up of rubocop#4796 (comment). Actually, it is only `with_index` method that accepts an offset argument. `with_with_index` method doesn't accept offset argument. However, I think that the same AST will be low maintenance cost.
Follow up of #4796 (comment). Actually, it is only `with_index` method that accepts an offset argument. `with_with_index` method doesn't accept offset argument. However, I think that the same AST will be low maintenance cost.
Follow up for #4708 (review).
This PR adds a cop @rrosenblum kindly suggested to me.
Feature
This cop checks for unneeded
with_object
.Before submitting the PR make sure the following are checked:
[Fix #issue-number]
(if the related issue exists).master
(if not - rebase it).rake spec
) are passing.rake internal_investigation
.and description in grammatically correct, complete sentences.
rake generate_cops_documentation
(required only when you've added a new cop or changed the configuration/documentation of an existing cop).