-
-
Notifications
You must be signed in to change notification settings - Fork 266
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 #623] Skip method shadowing check if attr is not sym or str for ReadWriteAttribute #619
Conversation
|
||
node.each_ancestor(:def).any? do |enclosing_method| |
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.
Unrelated to this PR.
Wondering, why we need to check all ancestors, or is it sufficient to check if the first one:
- exists
- matches the name
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.
Actually you are right, we do indeed interested in the first def
ancestor
I guess any?
is an implicit way of checking both exists and matches the way
Let me try to make it more explicit!
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.
Really good catch, I've changed it! Thanks ❤️
42ea0cd
to
1f79303
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.
Perfect, thank you!
Would you please add a changelog entry?
|
||
shadowing_method_name = first_arg.value.to_s | ||
shadowing_method_name << '=' if node.method?(:write_attribute) | ||
enclosing_method.method_name.to_s == shadowing_method_name |
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.
Can you use method?
?
enclosing_method.method_name.to_s == shadowing_method_name | |
enclosing_method.method?(shadowing_method_name) |
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.
Yep, that works! Thanks!
6ae69af
to
cc72b73
Compare
Added a changelog entry |
cc72b73
to
3a25016
Compare
3a25016
to
c95b625
Compare
CHANGELOG.md
Outdated
@@ -2,6 +2,10 @@ | |||
|
|||
## master (unreleased) | |||
|
|||
### Bug fixes | |||
|
|||
* [#619](https://github.com/rubocop/rubocop-rails/pull/619): Fix method shadowing check for `Rails/ReadWriteAttribute` cop. ([@nvasilevski][]) |
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.
Can you write to the following instead of editing the changelog directly?
- Added an entry (file) to the changelog folder named
{change_type}_{change_description}.md
if the new code introduces user-observable changes. See changelog entry format for details.
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.
Done, sorry, should have done it from the beginning
CI seems to be read because of some github actions being down?
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.
It may be a GitHub Actions incident. I'll take a look and merge if it works. Thank you!
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.
It may be this one.
https://www.githubstatus.com/incidents/cypv026dr23w
c95b625
to
05a3e34
Compare
…tr for ReadWriteAttribute
05a3e34
to
0d2c43c
Compare
Thanks! |
Closes #623
Fixes rubocop exceptions like:
for read/write methods used within a method and using non-string or non-symbol argument names
for example:
So I decided to skip the "shadowing" check if we can't explicitly know the attribute name.
Later we may want to exclude cases like this because it's also shadowing and should be fairly easy to detect this one. But for now I decided to fix the exception and consider an improvement in a separate PR
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 runs RuboCop on its own code.{change_type}_{change_description}.md
if the new code introduces user-observable changes. See changelog entry format for details.and description in grammatically correct, complete sentences.