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 #8044] Change cop department name calculation #8490
Conversation
I like the proposed change. In the past I simply didn't envision that there would be more than one level of namespacing, but this clearly has happened regardless. :-) @rubocop-hq/rubocop-core any objections to changing this? |
Having a department like Could we look at a an opt-in way instead? For example, Also, we already have some algorithm to lookup Cops equivalents, it would be nice to tweak them so that they can understand that "Capybara/FeatureMethods" => "RSpec/Capybara/FeatureMethods" |
Another good thing is that we plan to do a major release (v2.0) of rubocop-rspec shortly after RuboCop v1.0 is released. Which would be a good opportunity to release a breaking change like this. Of course, that necessitates waiting with merging this change until right before the 1.0 release. Or, making an opt-in solution like suggested by @marcandre and rubocop-rspec can then just drop the fallback in our 2.0 release. |
@marcandre, those are nice points! However, if I understood you correctly, I wouldn't want to make it an opt-in solution. Because in such a case it would fix the immediate problem but still leave room for problems in the future. What would happen if two different gems would choose the same department name for their new cop? In such an optional approach it's a matter of time when some confusion happens again. On the contrary, in the proposed solution the only conflict that could happen is when there are two cops with exactly the same namespaces and names, which is probably pretty easy to detect early. As to the second point, yes, that might probably help a lot! I'll try to look into this later. |
Nice, thanks a lot for dealing with this long-standing issue! Currently, extensions (checked the infamous
Does it make sense to add an error message for config loader to notify that e.g. |
decf236
to
fffe4f1
Compare
Okay, so I tried to do something to address the raised concerns. At first, I wasn't able to write some general code that would catch all the 'undeepened' names and convert them to 'deepened' ones. It really seemed to be pretty hard. So, this new So, what do you guys think? |
Agree. Release this change with 1.0 (but not earlier), and we'll make sure |
1ced871
to
b8df9f6
Compare
Ok, so I rolled back it to the initial version of the changes. I think in this shape this can be merged when going for version 1.0. Please don't forget about this PR when doing 1.0, I have been waiting for this fix for quite some time :) |
I've tagged the PR accordingly, so we won't forget about 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.
Thank you! Elegant solution 👏
To avoid name and department clash issues, RuboCop decided to grant each extension its own department. More info rubocop/rubocop#8490
We are ready for the change: |
b8df9f6
to
58939a0
Compare
To avoid name and department clash issues, RuboCop decided to grant each extension its own department. For those cops that already have the department matching the extension name, no changes are needed. More info rubocop/rubocop#8490
lib/rubocop/cop/badge.rb
Outdated
attr_reader :department, :cop_name | ||
|
||
def self.for(class_name) | ||
new(*class_name.split('::').last(2)) | ||
parts = class_name.split('::') | ||
parts_without_first_two = parts[2..-1] |
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 we come up with a better name for this variable?
Actually, I wonder if what we are really checking is whether the class name is a subclass of RuboCop::Cop
/RuboCop::Base
? In that case, I’d suggest a more explicit namespace check:
parts = class_name.split('::')
if class_name.start_with?('RuboCop::Cop', 'RuboCop::Base')
new(parts[2..-1])
else
new(parts.last(2))
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.
Thank could also be MyCoolCompany::Cop::Mailers::ExplicitRecipient
.
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.
How about e.g. MyCoolCompanyCops::Mailers::ExplicitRecipient
? It seems a bit arbitrary to decide that the first two namespaces are not part of the department name.
Even worse, how about MyCoolCompany::Cop::Style::Alias
? Should we would allow people’s internal cops to conflict with our cops.
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.
Also, as an experiment, I've moved one of the rubocop-rspec
cops up the hierarchy, and removed its department altogether. Everything went quite smoothly.
I'm not aware if the department is mandatory for cops, but if it worked previously, I believe it should continue to work.
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 we come up with a better name for this variable?
Actually we can get rid of this variable. I'm gonna update the PR.
Actually, I wonder if what we are really checking is whether the class name is a subclass of
RuboCop::Cop/RuboCop::Base? In that case, I’d suggest a more explicit namespace check:
Yes, if we are to change the code to the one proposed by @bquorning above, then we would still possibly have the same problem for cops MyCoolCompany::Cop::Mailers::ExplicitRecipient
and MyCoolCompany::Cop::Rails::Mailers::ExplicitRecipient
.
It seems a bit arbitrary to decide that the first two namespaces are not part of the department name.
I do understand that it is arbitrary to decide the first two namespaces are not part of the department name but I don't see a better solution right now, and this particular solution I'm proposing fixes the problem for all the current cases I think.
Even worse, how about MyCoolCompany::Cop::Style::Alias? Should we would allow people’s internal cops to
conflict with our cops.
That's a problem. But maybe a different one? Once and for all this can be fixed only if you put every part of this cop class name into its department/class name, something like MyCoolCompany/Cop/Style/Alias
. It would require everyone to change every existing cop. I am not happy proposing such a huge change :) Can this be fixed another way?
|
||
include_examples 'cop class name parsing', 'Foo', %w[Foo] | ||
include_examples 'cop class name parsing', 'Foo::Bar', %w[Foo Bar] | ||
include_examples 'cop class name parsing', 'RuboCop::Cop::Foo', %w[Cop Foo] |
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 think this line is wrong. We don’t want the class name "RuboCop::Cop::Foo"
to be recognized as a cop Foo
with department Cop
. Since the string starts with RuboCop::Cop
, Foo
must clearly be the department name, and the cop’s name is missing. I would expect Badge.for
to raise an error.
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.
Or, the department should be detected as none, and the cop name as Foo
.
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.
Well, I think this is exactly how it was before, it just wasn't documented here clearly. Previously method for
was new(*class_name.split('::').last(2))
so it would parse RuboCop::Cop::Foo
as Cop/Foo
(Cop
department).
If I am to add something like this to for
method
if class_name.start_with?('RuboCop::Cop', 'RuboCop::Base') && parts.length == 3
raise
end
it would break a lot of specs. For example, we have spec/rubocop/cop/cop_spec.rb and in that file we have the following:
RSpec.describe RuboCop::Cop::Cop, :config do
context 'with no submodule' do
it('has right name') { expect(cop_class.cop_name).to eq('Cop/Cop') }
it('has right department') { expect(cop_class.department).to eq(:Cop) }
end
end
So... it seems it was intentional it doesn't raise an error?
So I can add whatever thing you like here but maybe let's confirm it with the other core developers first since it would be a big change?
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 guess this was necessary during the period in which we had to support cops without a department (originally there were none and all cops were in the same namespace so to speak). I agree this should be changed.
06af666
to
4fa0aa4
Compare
@dmytro-savochkin Can you please rebase on the current |
81940cb
to
0d7c09b
Compare
0d7c09b
to
fcea71a
Compare
@bbatsov sure, rebased. |
Thanks! |
To avoid name and department clash issues, RuboCop decided to grant each extension its own department. For those cops that already have the department matching the extension name, no changes are needed. More info rubocop/rubocop#8490
To avoid name and department clash issues, RuboCop decided to grant each extension its own department. For those cops that already have the department matching the extension name, no changes are needed. More info rubocop/rubocop#8490 The changed cop names are: * `Capybara/CurrentPathExpectation` -> `RSpec/Capybara/CurrentPathExpectation` * `Capybara/FeatureMethods` -> `RSpec/Capybara/FeatureMethods` * `Capybara/VisibilityMatcher` -> `RSpec/Capybara/VisibilityMatcher` * `FactoryBot/AttributeDefinedStatically` -> `RSpec/FactoryBot/AttributeDefinedStatically` * `FactoryBot/CreateList` -> `RSpec/FactoryBot/CreateList` * `FactoryBot/FactoryClassName` -> `RSpec/FactoryBot/FactoryClassName` * `Rails/HttpStatus` -> `RSpec/Rails/HttpStatus`
To avoid name and department clash issues, RuboCop decided to grant each extension its own department. For those cops that already have the department matching the extension name, no changes are needed. More info rubocop/rubocop#8490 The changed cop names are: * `Capybara/CurrentPathExpectation` -> `RSpec/Capybara/CurrentPathExpectation` * `Capybara/FeatureMethods` -> `RSpec/Capybara/FeatureMethods` * `Capybara/VisibilityMatcher` -> `RSpec/Capybara/VisibilityMatcher` * `FactoryBot/AttributeDefinedStatically` -> `RSpec/FactoryBot/AttributeDefinedStatically` * `FactoryBot/CreateList` -> `RSpec/FactoryBot/CreateList` * `FactoryBot/FactoryClassName` -> `RSpec/FactoryBot/FactoryClassName` * `Rails/HttpStatus` -> `RSpec/Rails/HttpStatus`
To avoid name and department clash issues, RuboCop decided to grant each extension its own department. For those cops that already have the department matching the extension name, no changes are needed. More info rubocop/rubocop#8490 The changed cop names are: * `Capybara/CurrentPathExpectation` -> `RSpec/Capybara/CurrentPathExpectation` * `Capybara/FeatureMethods` -> `RSpec/Capybara/FeatureMethods` * `Capybara/VisibilityMatcher` -> `RSpec/Capybara/VisibilityMatcher` * `FactoryBot/AttributeDefinedStatically` -> `RSpec/FactoryBot/AttributeDefinedStatically` * `FactoryBot/CreateList` -> `RSpec/FactoryBot/CreateList` * `FactoryBot/FactoryClassName` -> `RSpec/FactoryBot/FactoryClassName` * `Rails/HttpStatus` -> `RSpec/Rails/HttpStatus`
To avoid name and department clash issues, RuboCop decided to grant each extension its own department. For those cops that already have the department matching the extension name, no changes are needed. More info rubocop/rubocop#8490 The changed cop names are: * `Capybara/CurrentPathExpectation` -> `RSpec/Capybara/CurrentPathExpectation` * `Capybara/FeatureMethods` -> `RSpec/Capybara/FeatureMethods` * `Capybara/VisibilityMatcher` -> `RSpec/Capybara/VisibilityMatcher` * `FactoryBot/AttributeDefinedStatically` -> `RSpec/FactoryBot/AttributeDefinedStatically` * `FactoryBot/CreateList` -> `RSpec/FactoryBot/CreateList` * `FactoryBot/FactoryClassName` -> `RSpec/FactoryBot/FactoryClassName` * `Rails/HttpStatus` -> `RSpec/Rails/HttpStatus`
Puppetlabs archived the https://github.com/puppetlabs/rubocop-i18n repo. This means puppetlabs/rubocop-i18n#45 will not be resolved. How do we move on? @dmytro-savochkin or @mvz maybe? |
I've thought about it since puppetlabs/rubocop-i18n#46 wasn't getting merged. Ideally, such a fork would live under the rubocop-hq organization. If the current owners are willing to grant access, maintainership of the gem could just be taken over. An alternative is to split the gem into a RailsI18n part and a GetText part, with new gem names. This may be a cleaner design since it would remove the need to activate one or the other i18n method. Is there some better place to discuss this further? |
@mvz You can open a new issue about this. I'd be happy to host the old gem or an alternative solution under |
@bbatsov done! |
To avoid name and department clash issues, RuboCop decided to grant each extension its own department. For those cops that already have the department matching the extension name, no changes are needed. More info rubocop/rubocop#8490 The changed cop names are: * `Capybara/CurrentPathExpectation` -> `RSpec/Capybara/CurrentPathExpectation` * `Capybara/FeatureMethods` -> `RSpec/Capybara/FeatureMethods` * `Capybara/VisibilityMatcher` -> `RSpec/Capybara/VisibilityMatcher` * `FactoryBot/AttributeDefinedStatically` -> `RSpec/FactoryBot/AttributeDefinedStatically` * `FactoryBot/CreateList` -> `RSpec/FactoryBot/CreateList` * `FactoryBot/FactoryClassName` -> `RSpec/FactoryBot/FactoryClassName` * `Rails/HttpStatus` -> `RSpec/Rails/HttpStatus`
To avoid name and department clash issues, RuboCop decided to grant each extension its own department. For those cops that already have the department matching the extension name, no changes are needed. More info rubocop/rubocop#8490 The changed cop names are: * `Capybara/CurrentPathExpectation` -> `RSpec/Capybara/CurrentPathExpectation` * `Capybara/FeatureMethods` -> `RSpec/Capybara/FeatureMethods` * `Capybara/VisibilityMatcher` -> `RSpec/Capybara/VisibilityMatcher` * `FactoryBot/AttributeDefinedStatically` -> `RSpec/FactoryBot/AttributeDefinedStatically` * `FactoryBot/CreateList` -> `RSpec/FactoryBot/CreateList` * `FactoryBot/FactoryClassName` -> `RSpec/FactoryBot/FactoryClassName` * `Rails/HttpStatus` -> `RSpec/Rails/HttpStatus`
To avoid name and department clash issues, RuboCop decided to grant each extension its own department. For those cops that already have the department matching the extension name, no changes are needed. More info rubocop/rubocop#8490 The changed cop names are: * `Capybara/CurrentPathExpectation` -> `RSpec/Capybara/CurrentPathExpectation` * `Capybara/FeatureMethods` -> `RSpec/Capybara/FeatureMethods` * `Capybara/VisibilityMatcher` -> `RSpec/Capybara/VisibilityMatcher` * `FactoryBot/AttributeDefinedStatically` -> `RSpec/FactoryBot/AttributeDefinedStatically` * `FactoryBot/CreateList` -> `RSpec/FactoryBot/CreateList` * `FactoryBot/FactoryClassName` -> `RSpec/FactoryBot/FactoryClassName` * `Rails/HttpStatus` -> `RSpec/Rails/HttpStatus`
To avoid name and department clash issues, RuboCop decided to grant each extension its own department. For those cops that already have the department matching the extension name, no changes are needed. More info rubocop/rubocop#8490 The changed cop names are: * `Capybara/CurrentPathExpectation` -> `RSpec/Capybara/CurrentPathExpectation` * `Capybara/FeatureMethods` -> `RSpec/Capybara/FeatureMethods` * `Capybara/VisibilityMatcher` -> `RSpec/Capybara/VisibilityMatcher` * `FactoryBot/AttributeDefinedStatically` -> `RSpec/FactoryBot/AttributeDefinedStatically` * `FactoryBot/CreateList` -> `RSpec/FactoryBot/CreateList` * `FactoryBot/FactoryClassName` -> `RSpec/FactoryBot/FactoryClassName` * `Rails/HttpStatus` -> `RSpec/Rails/HttpStatus`
Fixes #8044.
This is a pretty deep change. As I described in the issue page (#8044 (comment)) I can't see a way to fix this problem without changing the way cop departments are computed. So I'm proposing this fix.
After the fix the conflicting cops
RuboCop::Cop::Rails::HttpStatus
andRuboCop::Cop::RSpec::Rails::HttpStatus
will belong toRails
andRSpec/Rails
departments, respectively. Previously they both belong toRails
department, thus the issue.The biggest problem with this solution that all the dependent gems that have deep nested cops would need to update their
config/default.yml
files to reflect the changes. For example, forrubocop-rspec
the following seven cops would need to be updated:Capybara/CurrentPathExpectation
->RSpec/Capybara/CurrentPathExpectation
Capybara/FeatureMethods
->RSpec/Capybara/FeatureMethods
Capybara/VisibilityMatcher
->RSpec/Capybara/VisibilityMatcher
FactoryBot/AttributeDefinedStatically
->RSpec/FactoryBot/AttributeDefinedStatically
FactoryBot/CreateList
->RSpec/FactoryBot/CreateList
FactoryBot/FactoryClassName
->RSpec/FactoryBot/FactoryClassName
Rails/HttpStatus
->RSpec/Rails/HttpStatus
The good news is that
rubocop-rspec
andrubocop-i18n
are probably the only gems that would need to be changed.This change should not affect any cops that have 4 or less levels of nesting. For example, some cop named
Rubocop::Cop::Foo::Bar
now has departmentFoo
and this will still be the case after this change. CopRubocop::Cop::Foo::Bar::Baz
, which now has departmentBar
, will have departmentFoo/Bar
after the change however.Due to the magnitude of this in the discussion in the comments we agreed to move forward with this when rubocop hits version 1.0.
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.