-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Enabling RSpec IndexedLet Rubocop rule #10024
Conversation
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.
Swapping out file1
for first_file
seems not in the spirit of the rule.
It makes reading the test harder because it’s not clear what exactly is tested by this particular example.
That is not addressed by changing things in this way.
There are also a bunch of scenarios here where v1
or v2
refers to the version of the package manager. We should configure a pattern to allow that, because that is very clear and the current changes actually make those cases much harder to understand, so we'd end up in a worse situation there.
@@ -14,7 +14,7 @@ | |||
LOCKFILE | |||
end | |||
|
|||
let(:lockfile_bundled_with_v1) do | |||
let(:first_version_lockfile) 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.
v1 refers to the version of Bundler here. Not it being the first occurrence of the variable.
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 can change that one to lockfile_bundle_with_version_one. It keeps the same spirit of that original name. In another fie I changed v6 to version_six for that reason.
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.
corrected with commit 4c83ce7. Please review
None of these changes seem like they are an improvement, the spirit of this linter rule is to make the tests more readable. All of the changes I see here are along the lines of changing If we can't think of descriptive names for these, I'd rather we disable the rule than make these changes. |
Instead of changing all of these files, per discussions, we are going to simply disable this rule as it does not gain us anything in the long run. |
Not exactly how I would put it, but if all we're doing is changing |
https://docs.rubocop.org/rubocop-rspec/cops_rspec.html#rspecindexedlet
What are you trying to accomplish?
The RSpec IndexedLet rubocop rule has been disabled. This PR will re-enable it on the whole ecosystem.
Anything you want to highlight for special attention from reviewers?
This was essentially a renaming of variables in the rspec files. It shoud not affect anything.
How will you know you've accomplished your goal?
This cop just asks us to rename variables. The tests should run as normal.
Checklist