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 #10919] Fix a huge performance regression between 1.32.0 and 1.33.0 #10953
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 |
---|---|---|
@@ -0,0 +1 @@ | ||
* [#10919](https://github.com/rubocop/rubocop/issues/10919): Fix a huge performance regression between 1.32.0 and 1.33.0. ([@ydah][]) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -951,29 +951,34 @@ def a; end | |
YAML | ||
end | ||
|
||
context 'when configurable parameter is an obsolete parameter' do | ||
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. This was a test added in #10875. However, this specification was incorrect. In fact, I think the cop_config was broken, causing duplicates to be displayed. 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. Isn't it necessary to update the test as originally intended? I'm worried about losing a regression spec for #10875. 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. Indeed it is. I will modify the test a bit and add it. 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. Hmmm. I've tried running the following test and it seems to fail. Maybe there is some other problem. context 'when duplicated parameter' do
it 'parameters are displayed without duplication' do
create_file('.rubocop.yml', <<~YAML)
AllCops:
NewCops: enable
Lint/AmbiguousBlockAssociation:
AllowedMethods:
- change
- not_change
- change
YAML
create_file('example1.rb', ['# frozen_string_literal: true', '', 'def fooBar; end'])
create_file('example2.rb', ['# frozen_string_literal: true', '', 'def fooBar; end'])
expect(cli.run(['--auto-gen-config'])).to eq(0)
File.readlines('.rubocop_todo.yml')
expect(File.readlines('.rubocop_todo.yml')[9..].join)
.to eq(<<~YAML)
# Configuration parameters: AllowedPatterns, IgnoredPatterns.
# SupportedStyles: snake_case, camelCase
# AllowedPatterns: change, not_change
Naming/MethodName:
EnforcedStyle: camelCase
YAML
end
end
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 feel like we need guarantees for existing behavior against performance regression solutions. I'm wondering whether another issue is included due to the test failing. 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 see. I would like to run the same test code on the old version to check the existing behavior first. Thank you. 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. As noted below, the display of this configuration value is the configuration value in |
||
it 'displayed as a new parameter setting value without duplication' do | ||
context 'when duplicated default configuration parameter' do | ||
before do | ||
RuboCop::ConfigLoader.default_configuration['Naming/MethodParameterName'] | ||
.merge!('AllowedNames' => %w[at by at]) | ||
end | ||
Comment on lines
+955
to
+958
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. The configuration values like |
||
|
||
it 'parameters are displayed without duplication' do | ||
create_file('.rubocop.yml', <<~YAML) | ||
AllCops: | ||
NewCops: enable | ||
Naming/MethodName: | ||
# `IgnoredPatterns` is obsolete. `AllowedPatterns` is newly used. | ||
IgnoredPatterns: | ||
- change | ||
- not_change | ||
Naming/VariableName: | ||
Enabled: false | ||
YAML | ||
create_file('example1.rb', ['# frozen_string_literal: true', '', 'def fooBar; end']) | ||
create_file('example2.rb', ['# frozen_string_literal: true', '', 'def fooBar; end']) | ||
create_file('example1.rb', <<~TEXT) | ||
# frozen_string_literal: true | ||
|
||
def bar(varOne, varTwo) | ||
varOne + varTwo | ||
end | ||
TEXT | ||
|
||
expect(cli.run(['--auto-gen-config'])).to eq(0) | ||
File.readlines('.rubocop_todo.yml') | ||
expect(File.readlines('.rubocop_todo.yml')[9..].join) | ||
.to eq(<<~YAML) | ||
# Configuration parameters: AllowedPatterns, IgnoredPatterns. | ||
# SupportedStyles: snake_case, camelCase | ||
# AllowedPatterns: change, not_change | ||
Naming/MethodName: | ||
EnforcedStyle: camelCase | ||
# Configuration parameters: MinNameLength, AllowNamesEndingInNumbers, AllowedNames, ForbiddenNames. | ||
# AllowedNames: at, by | ||
Naming/MethodParameterName: | ||
Exclude: | ||
- 'example1.rb' | ||
YAML | ||
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.
[memo] The reason for the slow speed was due to this method.
If the following settings were used
it was returning the following results (increasing with each file)