-
Notifications
You must be signed in to change notification settings - Fork 50
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
Implement UnusedArgument cop #61
Implement UnusedArgument cop #61
Conversation
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.
Good start! Please check out my comments below and let's make CI pass
Co-authored-by: Dmitry Tsepelev <dmitry.a.tsepelev@gmail.com>
Co-authored-by: Dmitry Tsepelev <dmitry.a.tsepelev@gmail.com>
Co-authored-by: Dmitry Tsepelev <dmitry.a.tsepelev@gmail.com>
Co-authored-by: Dmitry Tsepelev <dmitry.a.tsepelev@gmail.com>
Thanks for the comments! I've applied them, and also made the code backward compatible. I am not sure if the following method if reliable enough: def method_name(node)
node.loc.keyword.end.resize(node.method_name.to_s.size + 1)
end
|
Specs are still failing (because of my stupid suggestion mostly) and we have a number of rubocop offences 🙂 |
Ok, I fixed it all 🤞 |
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.
LGTM, thank you!
Was the
but that complains that it wants :) |
No, it was not. I will fix it, thanks! |
I've created the fix #65 |
First of all, I've implemented autocorrection, and I am not sure if it is better than just add errors to arguments.
And the second thing is the cop ignores the case when there is no
resolve
method at all.What do you think?
Resolves #53