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 #7680] Add new Lint/StructNewOverride
cop
#7699
Conversation
By following #7680 (comment), I've mad the cop message milder via f10bc600aa3f96e962bce9b7d9d4822d2a48eb8a. E.g.
|
# g.clone #=> #<struct Good id=1, name="foo"> | ||
# g.count #=> 2 | ||
# | ||
class StructNewOverride < 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.
And then we can name this StructBuiltInMethodOverride
which is more descriptive IMO.
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.
Thank you. I also think that StructBuiltInMethodOverride
is easier to understand than StructNewOverride
. 👍
I'll fix 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.
I assume that this cop detects some overrides by arguments of Struct.new
, not normal overrides as follows:
Foo = Struct.new(:name) do
def to_s
name.to_s
end
end
I'm a bit concerned that StructBuiltInMethodOverride
may be considered as any overrides of the built-in methods. 🤔
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.
For example, StructBuiltInMethodOverrideByConstructor
may be more descriptive but too long. 😓
I may think too much... Please give me any help! 🙏
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.
Naming is hard! :D Let's keep the shorter name and down the road we can maybe extend this cop to cover dangerous overrides by explicit definitions as well.
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.
Make sense. Thanks! 👍
Apart from my small remarks - great work! |
I've addressed all the reviews. Please review again if you have time. 🙏 |
🚀 |
Fix #7680
This adds a new
Lint/StructNewOverride
cop to check unexpected overrides ofStruct
original methods.I originally planned to name it "Lint/StructOverride", but I think that disallowing all the overrides is too strict, so I rename it to
Lint/StructNewOverride
. I intend that this cop limits to overrides via arguments inStruct.new(...)
.However, the cop name may need to be discussed more.
Furthermore, as I commented on #7680 (comment), it may need more discussion on whether the cop should check all the methods (from
Object
orEnumerable
) or not.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.