-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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 #11696] Add new Style/DataInheritance
cop
#11728
Conversation
module RuboCop | ||
module Cop | ||
module Style | ||
# Checks for inheritance from Data.define. |
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'd also mention here what's the problem we're trying to solve (avoid the creation of the anonymous parent class).
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.
Added couple words, thanks for the comment! I was considering adding outputs of .ancestors
and whatnot, but was afraid it will be too verbose then. WDYT about current form?
It would also be nice to add a matching guideline in the Ruby style guide and mention it in the cop's metadata. |
I'm referring to https://github.com/rubocop/ruby-style-guide#dont-extend-structnew |
|
a295679
to
022a32e
Compare
See also: * [PR adding new cop for this rule](rubocop/rubocop#11728) * [Original issue](rubocop/rubocop#11696)
5175dd7
to
da921aa
Compare
Thanks for the comments @koic and @bbatsov ! I've added the |
See also: * [PR adding new cop for this rule](rubocop/rubocop#11728) * [Original issue](rubocop/rubocop#11696)
|
||
RSpec.describe RuboCop::Cop::Style::DataInheritance, :config do | ||
context 'Ruby >= 3.2', :ruby32 do | ||
it 'registers an offense when extending instance of Data' do |
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 replace the entire test description with Data.define
?
it 'registers an offense when extending instance of Data' do | |
it 'registers an offense when extending instance of `Data.define`' do |
end | ||
RUBY | ||
end | ||
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.
Can you add a test for context 'Ruby <= 3.1', :ruby31 do
that does not register an offense?
end | ||
|
||
# @!method data_constructor?(node) | ||
def_node_matcher :data_constructor?, <<~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.
def_node_matcher :data_constructor?, <<~PATTERN | |
def_node_matcher :data_define?, <<~PATTERN |
module RuboCop | ||
module Cop | ||
module Style | ||
# Checks for inheritance from Data.define to avoid creating the anonymous parent class. |
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.
# Checks for inheritance from Data.define to avoid creating the anonymous parent class. | |
# Checks for inheritance from `Data.define` to avoid creating the anonymous parent class. |
@ktopolski It'd be nice to wrap this up soon, so it can be included in the next RuboCop release. |
eb41502
to
bc42301
Compare
Thanks! |
Fixes #11696.
This PR adds new cop:
Style/DataInheritance
. It's based on theStyle/StructInheritance
cop.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.