Skip to content

Introduce RuboCop Performance#8024

Merged
tagliala merged 8 commits into
activeadmin:masterfrom
tagliala:chore/introduce-rubocop-performance
Oct 24, 2023
Merged

Introduce RuboCop Performance#8024
tagliala merged 8 commits into
activeadmin:masterfrom
tagliala:chore/introduce-rubocop-performance

Conversation

@tagliala

@tagliala tagliala commented Jul 23, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Commits are separated by scope:

  • Add gem
  • Autofix safe, non-production offenses
  • Autofix unsafe, non-production offenses
  • Autofix safe, production offenses
  • Autofix unsafe, production offenses
  • Manually fix remaining production offenses

Notes:

  • All changed lines of code are covered by specs (required Remove multiline ternary operator #8034)
  • Performance/EndWith changes should be safe because minimum required ruby version is 2.7, and 2.7 also supports Symbol#end_with?
  • Performance/InefficientHashSearch fixes should be safe, because both @menus and @children are hashes
  • Performance/StringInclude fixes should be safe, because @column is the result of a RegExp match
  • changelog:resync and local rake tasks have been tested manually

@codecov

codecov Bot commented Jul 23, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6d85e1f) 99.05% compared to head (266aacb) 99.05%.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #8024   +/-   ##
=======================================
  Coverage   99.05%   99.05%           
=======================================
  Files         184      184           
  Lines        4738     4740    +2     
=======================================
+ Hits         4693     4695    +2     
  Misses         45       45
Files Coverage Δ
features/step_definitions/table_steps.rb 100.00% <100.00%> (ø)
lib/active_admin/application.rb 98.97% <100.00%> (ø)
lib/active_admin/callbacks.rb 96.42% <100.00%> (+0.13%) ⬆️
lib/active_admin/dsl.rb 100.00% <100.00%> (ø)
lib/active_admin/filters/forms.rb 100.00% <100.00%> (ø)
lib/active_admin/filters/resource_extension.rb 97.46% <100.00%> (ø)
lib/active_admin/form_builder.rb 100.00% <100.00%> (ø)
lib/active_admin/menu.rb 97.43% <100.00%> (ø)
lib/active_admin/menu_collection.rb 100.00% <100.00%> (ø)
lib/active_admin/namespace.rb 100.00% <100.00%> (ø)
... and 11 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tagliala tagliala force-pushed the chore/introduce-rubocop-performance branch 2 times, most recently from a14ec53 to d486420 Compare July 28, 2023 08:37
@tagliala tagliala force-pushed the chore/introduce-rubocop-performance branch 2 times, most recently from 3bd7398 to b36bd2d Compare October 7, 2023 16:58
@tagliala tagliala changed the title [WIP] Introduce RuboCop Performance Introduce RuboCop Performance Oct 7, 2023
@tagliala tagliala marked this pull request as ready for review October 7, 2023 17:02
@tagliala tagliala force-pushed the chore/introduce-rubocop-performance branch from b36bd2d to 4041cb4 Compare October 13, 2023 16:43
javierjulio
javierjulio reviewed Oct 14, 2023
View reviewed changes

@javierjulio javierjulio left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tagliala thank you again for this. I've implemented rubocop-performance at work so I'm familiar with it. It's been a helpful tool. The main concern I have is with what was auto corrected vs what is reported in future updates.

With our config, there are some changes here that shouldn't have occurred since the cops are disabled by default. I believe if you auto correct by rule manually that will override our config settings. Reviewing your commits I believe it was auto correct by rule.

Can you please declare each rubocop-performance cop in the config and whether it should be enabled or not? This is important as we don't want to auto correct something here that isn't being reported as a warning in future updates. I've included a copy of all the cops config so you don't have to write out each one. Please change the values for each as needed.

Comment thread lib/active_admin/resource_controller/streaming.rb Outdated
Comment thread spec/unit/views/components/paginated_collection_spec.rb
Comment thread .rubocop.yml
@tagliala tagliala marked this pull request as draft October 15, 2023 06:58
@tagliala tagliala force-pushed the chore/introduce-rubocop-performance branch from 4041cb4 to 65b1224 Compare October 18, 2023 09:22
@tagliala tagliala requested a review from javierjulio October 18, 2023 09:24
@tagliala tagliala marked this pull request as ready for review October 18, 2023 09:31
@tagliala

tagliala commented Oct 18, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Failure on updating code coverage does not seem to depend by these changes

[2023-10-18T09:31:27.490Z] ['error'] There was an error running the uploader: Error uploading to [https://codecov.io:](https://codecov.io/) Error: There was an error fetching the storage URL during POST: 404 - {'detail': ErrorDetail(string='Unable to locate build via Github Actions API. Please upload with the Codecov repository upload token to resolve issue.', code='not_found')}

Temporary failures

@tagliala tagliala force-pushed the chore/introduce-rubocop-performance branch 2 times, most recently from 7acbb5d to 9ff24bb Compare October 24, 2023 09:01
javierjulio
javierjulio approved these changes Oct 24, 2023
View reviewed changes

@javierjulio javierjulio left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @tagliala! Just two minor changes. This is ready to merge but has a single merge conflict.

Comment thread .rubocop.yml Outdated
Comment thread .rubocop.yml
Comment thread lib/active_admin/resource_controller/streaming.rb Outdated
@tagliala
Introduce RuboCop performance
7e4e32c 
Also fixes issues in non-production code
@tagliala
Fix safe offenses in non-production code
7d776df 
- Performance/CompareWithBlock
- Performance/RegexpMatch
- Performance/StringReplacement
@tagliala
Fix unsafe offenses in non production code
e558d28 
- Performance/Count
- Performance/StringInclude
- Performance/TimesMap
@tagliala
Fix safe production RuboCop Performance offenses
ad95f51 
- Performance/BlockGivenWithExplicitBlock
- Performance/RedundantBlockCall
- Performance/RegexpMatch
- Performance/StringIdentifierArgument
- Performance/StringReplacement
@tagliala
Fix unsafe offenses in production code
912317f 
- Performance/EndWith
- Performance/InefficientHashSearch
- Performance/StringInclude
@tagliala
Fix performance offenses manually
f4a3c20 
Fix offenses that cannot be automatically corrected:
- Performance/CollectionLiteralInLoop
- Performance/MethodObjectAsBlock
@tagliala tagliala force-pushed the chore/introduce-rubocop-performance branch 2 times, most recently from 2a7c72c to cdd08b4 Compare October 24, 2023 20:07
tagliala
tagliala commented Oct 24, 2023
View reviewed changes
Comment thread spec/unit/views/pages/form_spec.rb
@tagliala
Disable CollectionLiteralInLoop in specs
cdd770a 
Allow Performance/CollectionLiteralInLoop in specs

There are four offenses that may be solved in the future
@tagliala
Remove OpenStruct
266aacb
@tagliala tagliala force-pushed the chore/introduce-rubocop-performance branch from cdd08b4 to 266aacb Compare October 24, 2023 21:06
tagliala
tagliala commented Oct 24, 2023
View reviewed changes
Comment thread .rubocop.yml
@javierjulio

javierjulio commented Oct 24, 2023

Copy link
Copy Markdown
Member

@tagliala this is ready for you to merge. Thank you again for contributing this! ❤️

@tagliala

tagliala commented Oct 24, 2023

Copy link
Copy Markdown
Contributor Author

@javierjulio welcome. Should I use squash & merge for this PR?

@javierjulio

javierjulio commented Oct 24, 2023

Copy link
Copy Markdown
Member

@tagliala yes

@tagliala tagliala merged commit a794c9a into activeadmin:master Oct 24, 2023
@tagliala tagliala deleted the chore/introduce-rubocop-performance branch October 24, 2023 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@javierjulio javierjulio javierjulio approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@tagliala @javierjulio