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
Add new EnableNewCopsUpTo
option to enable all pending cops up to a specific Rubocop version
#8565
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -390,7 +390,8 @@ class SomeCop < Cop | |
|
||
context 'when Style department is enabled' do | ||
let(:new_cops_option) { '' } | ||
let(:version_added) { "VersionAdded: '0.80'" } | ||
let(:version_added) { "VersionAdded: '1.80'" } | ||
let(:department_option) { '' } | ||
|
||
before do | ||
create_file('.rubocop.yml', <<~YAML) | ||
|
@@ -400,6 +401,10 @@ class SomeCop < Cop | |
DefaultFormatter: progress | ||
#{new_cops_option} | ||
|
||
Style: | ||
Enabled: true | ||
#{department_option} | ||
|
||
Style/SomeCop: | ||
Description: Something | ||
Enabled: pending | ||
|
@@ -415,12 +420,39 @@ class SomeCop < Cop | |
remaining_range = pending_cop_warning.length..-(inspected_output.length + 1) | ||
pending_cops = output[remaining_range] | ||
|
||
expect(pending_cops).to include("Style/SomeCop: # new in 0.80\n Enabled: true") | ||
expect(pending_cops).to include("Style/SomeCop: # new in 1.80\n Enabled: true") | ||
|
||
manual_url = output[remaining_range].split("\n").last | ||
|
||
expect(manual_url).to eq(versioning_manual_url) | ||
end | ||
|
||
context "when the department's NewCops option is a version > VersionAdded" do | ||
let(:department_option) { "NewCops: '1.81'" } | ||
|
||
it 'does not display a pending cop warning for that cop' do | ||
expect(output).to start_with(pending_cop_warning) | ||
expect(output).not_to include("Style/SomeCop: # new in 1.80\n Enabled: true") | ||
end | ||
end | ||
|
||
context "when the department's NewCops option is a version < VersionAdded" do | ||
let(:department_option) { "NewCops: '1.79'" } | ||
|
||
it 'does display a pending cop warning for that cop' do | ||
expect(output).to start_with(pending_cop_warning) | ||
expect(output).to include("Style/SomeCop: # new in 1.80\n Enabled: true") | ||
end | ||
end | ||
|
||
context "when the department's NewCops option is a version == VersionAdded" do | ||
let(:department_option) { "NewCops: '1.80'" } | ||
|
||
it 'does not display a pending cop warning for that cop' do | ||
expect(output).to start_with(pending_cop_warning) | ||
expect(output).not_to include("Style/SomeCop: # new in 1.80\n Enabled: true") | ||
end | ||
end | ||
Comment on lines
+430
to
+455
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updating these tests for Rubocop 1.0+ highlighted how fragile they are:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
You probably could stub out the version constant for the tests. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree though that these tests are brittle, it’d be good if we could generate a data structure of cops that will be outputted and use that both to output and to test against, rather than testing the output itself. At the very least, instead of |
||
end | ||
|
||
context 'when `VersionAdded` is not specified' 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.
I believe "extension" is a more appropriate term.
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 believe that extensions and departments are orthogonal. An extension can add cops to a department that exists in the core RuboCop.
Making this as flexible as allowing to configure it per-extension and per department is an overkill.
AllCops/NewCops
for RuboCop core and optional separate per-extension is just fine.WDYT:
equivalent to:
For extensions:
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.
Setting a version number, is ambiguous.
Should a version of RuboCop core or an extension that introduced a certain cop to the existing department be used to decide if the cop is affirmed or not?
I believe some kind of warning should be issued in case of such an ambiguity, e.g.:
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 a problem will be we don’t know what extension adds what cop but I agree per extension makes more sense to me than per dept.