Add RuboCop Performance #5586
Merged
Add RuboCop Performance #5586
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Disabled |
|
With the current slate of cops, we get: Of the remaining after autocorrection: If we exclude If these seem reasonable, we can go through and address them one-by-one. |
|
The suggested cops seem fine, also they lead to more readable code in my opinion, so not only performance gains! I wonder why |
jeffwidman
reviewed
Aug 29, 2022
mattt
force-pushed
the
mattt/rubocop-performance
branch
from
3d298ab
to
9d49549
Compare
mattt
force-pushed
the
mattt/rubocop-performance
branch
from
7fc81d0
to
91e54e3
Compare
|
TIL:
|
mattt
force-pushed
the
mattt/rubocop-performance
branch
7 times, most recently
from
ae78e5d
to
554cb3e
Compare
deivid-rodriguez
approved these changes
Sep 3, 2022
There was a problem hiding this comment.
I really love these, I should use RuboCop more 😄.
Only cop I'm "meh" about is Performance/CaseCmp, because while I think the other cops make the code better (not only faster), this one I don't love it, because it leaks implementation details about the comparison into our code (although I admit well known for most: zero means equality).
As a tiny note, a9aa5e86c9a5c728105d4748784f4fe908343b94 mentions "Disable Performance/ArraySemiInfiniteRangeSlice", but I don't see that cop anywhere?
Anyways, thanks for doing this!
| next 0 unless all_parts.all? { |part| part.to_i.to_s == part } | ||
| # rubocop:enable Performance/RedundantEqualityComparisonBlock |
There was a problem hiding this comment.
This sounds like an upstream false positive, right? I can report it if so.
There was a problem hiding this comment.
Yes, that's a false positive. It suggests an incorrect fix of all_parts.all?(part.to_i.to_s).
| relative_dir = Pathname.new(base_directory).sub(%r{\A/}, "").join(vendor_dir) | ||
| # rubocop:enable Performance/DeletePrefix |
There was a problem hiding this comment.
This one may be skippable upstream too, and also Pathname#delete_prefix sounds like a nice feature request. I can take of requesting both if they make sense.
There was a problem hiding this comment.
Yeah, there's its complement Performance/DeleteSuffix has a similar problem.
It would be great if RuboCop didn't generate a false positive for Pathname. In rubocop/rubocop-performance#245, they marked it as not safe, but I don't know of any other changes beyond that.
Conversely, it'd also be nice if Pathname responded to #delete_prefix and #delete_suffix. I was surprised those methods didn't exist already.
| when *Octokit::RATE_LIMITED_ERRORS | ||
| # If we get a rate-limited error we let dependabot-api handle the | ||
| # retry by re-enqueing the update job after the reset |
There was a problem hiding this comment.
Is removing the above comment intentional?
There was a problem hiding this comment.
Good catch! Those must've been lost in the shuffle when the splat was moved to the end.
Require rubocop-performance in configuration Disable Performance/OpenStruct Disable Performance/SelectMap Disable Performance/ChainArrayAllocation Disable Style/MultipleComparison
Disable Performance/RedundantEqualityComparisonBlock in pull_request/labeler.rb
Disable false positives for Performance/DeletePrefix
Disable Performance/MethodObjectAsBlock in mixfile_sanitizer.rb
Fix test failures introduced by &block signature changes
mattt
force-pushed
the
mattt/rubocop-performance
branch
from
554cb3e
to
d00b8aa
Compare
Nice call-out. I reverted that commit after reading the docs for
I'm not the biggest fan of Edit: Actually, fact-checking that last statement, I realized that @deivid-rodriguez Thanks for your offer to upstream fixes for the false positives. I think that'd be a great thing to do. Please let me know how I can help. 😃 |
|
@mattt Ok about Regarding false positives, my offer was to report them, not fix them 😅, but my experience when reporting this kind of issues upstream is that they usually fix them quickly, so we'll see :) |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related to #5588
RuboCop Performance is a RuboCop extension that adds linters for Ruby (anti-)patterns that have an outsized performance impact.
This PR adds
rubocop-performanceas a development dependency todependabot-commonand updates the shared.rubocop.ymlfile to enable Performance cops.