Skip to content
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

Extract Performance Cops into a separate gem #5977

Closed
bbatsov opened this issue Jun 9, 2018 · 25 comments
Closed

Extract Performance Cops into a separate gem #5977

bbatsov opened this issue Jun 9, 2018 · 25 comments
Labels
help wanted high priority A ticket considered important by RuboCop's Core Team refactoring
Milestone

Comments

@bbatsov
Copy link
Collaborator

bbatsov commented Jun 9, 2018

Performance cops have to extracted into a standalone gem (named something like rubocop-performance or whatever).

The reason for this decision is simple. I want RuboCop to be focused just on Ruby and have no framework/library/runtime dependant pieces for the sake of being easier to focus on evolving it. Creating more extensions should also help us come up with a good official API for them. Performance cops are all MRI focused and are highly dependent of the version of MRI you're using, so they don't belong in the core RuboCop.

Any volunteers to do this? I was always ambivalent towards the performance checks (as they often come at the expense of readability), so I'm not particularly interested in working on this.

@bbatsov bbatsov added this to the 1.0.0 milestone Jun 9, 2018
@Drenmi
Copy link
Collaborator

Drenmi commented Jun 9, 2018

Any volunteers to do this? I was always ambivalent towards the performance checks (as they often come at the expense of readability), so I'm not particularly interested in working on this.

Agreed. Language level optimizations also suffer from being subject to the current VM implementation. So a big task for whoever wants to take this would be to benchmark on every Ruby version and see that the improvements still hold.

@bbatsov
Copy link
Collaborator Author

bbatsov commented Jun 9, 2018

Although they just extract it as-is in the beginning and improve things later. Probably performance cops should have explicit versions on which they make sense as part of their config.

@gizotti
Copy link

gizotti commented Jun 10, 2018

Extracting as it is into a gem seems simple enough. I could handle this one if nobody is on it yet.

I also like the idea of showing instructions on how to add the gem if they have performance cops in their configs, the same way you suggested on #5976.

@bbatsov
Copy link
Collaborator Author

bbatsov commented Jun 10, 2018

@gizotti Happy to hear this!

I've created a placeholder repo for the extraction https://github.com/rubocop-hq/rubocop-performance

@gizotti
Copy link

gizotti commented Jun 10, 2018

@bbatsov can you just add a README on the repo please? I can't fork an empty repo :)

@bbatsov
Copy link
Collaborator Author

bbatsov commented Jun 10, 2018

Done.

@composerinteralia
Copy link
Contributor

composerinteralia commented Jun 10, 2018

Oh darn, I didn't see that you wanted to take this on @gizotti. I opened rubocop/rubocop-performance#1 but maybe we can work together on it.

If you are already further along than I was, I can also just close the PR. I should have checked the issue again before doing anything.

@gizotti
Copy link

gizotti commented Jun 10, 2018

No worries @composerinteralia, I got to where you were last night so not reason to push my stuff if you already did.
I'll leave you to it, I think if we both work in the same branch we might step on each others toes.

@bbatsov bbatsov added the high priority A ticket considered important by RuboCop's Core Team label Sep 12, 2018
@koic
Copy link
Member

koic commented Oct 1, 2018

I'm investigating the same git history problem as the current RuboCop Rails repository. The current situation is below.
#5976 (comment)

@koic
Copy link
Member

koic commented Oct 12, 2018

@bbatsov @composerinteralia
Cc @rubocop-hq/rubocop-core

I extracted lib/rubocop/cop/performance/* and spec/rubocop/cop/performance/* along with git history from RuboCop Core (I used git cherry-pick for this extraction) .
I think that lib/rubocop/cop/performance and spec/rubocop/cop/performance will be the most important history in maintenance.

The following branch including previous work done in RuboCop Performance repo, and it is synchronized with the latest performance cops of RuboCop Core.
https://github.com/koic/rubocop-performance/tree/import_performance_cops_with_git_history

If it's OK, force-push this branch to master of RuboCop Performance repo.
https://github.com/rubocop-hq/rubocop-performance

After that, we can restart work based on this git history for this issue.

📝 @amatsuda told me the experiecce that how to divided Kaminari repo at Asakusa.rb. So I was able to obtain great knowledge. Thank you!

@Drenmi
Copy link
Collaborator

Drenmi commented Oct 12, 2018

@koic, @amatsuda 🙇

@bbatsov
Copy link
Collaborator Author

bbatsov commented Oct 12, 2018

If it's OK, force-push this branch to master of RuboCop Performance repo.

Yeah, it's OK. Make this happen!

P.S. Great work, probably should document the process somewhere for future reference.

@koic
Copy link
Member

koic commented Oct 12, 2018

Thanks! I force-pushed it.
This was a very steady work 😅, but I'm thinking of share know-how somewhere.

@koic
Copy link
Member

koic commented Mar 13, 2019

@bbatsov Thank you for waiting. We can release the rubocop-performance gem.

We need to decide on the rubocop-performance version to be released. The version number should be a number greater than 0.61.
rubocop/rubocop-performance#26 (review)

Also, I'm ready to remove performance cops from rubocop-hq/rubocop repo, but before that we need to notify the users in some way.

Anyway, it is good to release the rubocop-performance gem.

@bbatsov
Copy link
Collaborator Author

bbatsov commented Mar 13, 2019

Fantastic work!

Let's go with version 1.0 for rubocop-performance.

@bbatsov
Copy link
Collaborator Author

bbatsov commented Mar 13, 2019

Also, I'm ready to remove performance cops from rubocop-hq/rubocop repo, but before that we need to notify the users in some way.

Let's just add some message that say on gem install that the cops are going to be moved to a different gem. Can't think of any better way to notify them about the change, so I hope this will be enough.

@koic
Copy link
Member

koic commented Mar 14, 2019

Thank you for the advice!
First I will release rubocop-performance 1.0. After that, I will open a PR to RuboCop core which will fallback to rubocop-performance with the message.

@bbatsov
Copy link
Collaborator Author

bbatsov commented Mar 14, 2019

👍

koic added a commit to koic/rubocop-performance that referenced this issue Mar 14, 2019
Follow up of rubocop/rubocop#5977 (comment).

This PR prepares for release 1.0.0.
@koic
Copy link
Member

koic commented Mar 14, 2019

rubocop-performance 1.0.0 has been released 🚀
https://rubygems.org/gems/rubocop-performance/versions/1.0.0

@bbatsov
Copy link
Collaborator Author

bbatsov commented Mar 14, 2019

Fantastic!

I'll make an announcement once we've made the necessary changes to RuboCop itself.

bbatsov pushed a commit that referenced this issue Apr 8, 2019
Fixes #5977.

This PR removes Performance cops from RuboCop core.

These cops were transferred to rubocop-hq/rubocop-performance repository.
https://github.com/rubocop-hq/rubocop-performance
koic added a commit to koic/rails that referenced this issue Apr 16, 2019
Performance cops will be extracted from RuboCop to RuboCop Performance
when next RuboCop 0.68 will be released.
rubocop/rubocop#5977

RuboCop 0.67 is its transition period.

Since rails/rails repository uses Performance cops, This PR added
rubocop-performance gem to Gemfile.

And this PR fixes some offenses using the following auto-correct.

```console
% bundle exec rubocop -a

Offenses:

activerecord/test/cases/connection_adapters/connection_handlers_multi_db_test.rb:212:26:
C: [Corrected] Layout/SpaceAroundOperators: Operator =
> should be surrounded by a single space.
              "primary"  => { adapter: "sqlite3", database: "db/primary.sqlite3" }
                         ^^
activerecord/test/cases/connection_adapters/connection_handlers_multi_db_test.rb:239:26:
C: [Corrected] Layout/SpaceAroundOperators: Operator => should be
surrounded by a single space.
              "primary"  => { adapter: "sqlite3", database: "db/primary.sqlite3" }
                         ^^
actionview/test/template/resolver_shared_tests.rb:1:1: C: [Corrected]
Style/FrozenStringLiteralComment: Missing magic comment #
frozen_string_literal: true.
module ResolverSharedTests
^
actionview/test/template/resolver_shared_tests.rb:10:33: C: [Corrected]
Layout/SpaceAroundEqualsInParameterDefault: Surrounding space missing in
default value assignment.
  def with_file(filename, source="File at #{filename}")
                                ^
actionview/test/template/resolver_shared_tests.rb:106:5: C: [Corrected]
Rails/RefuteMethods: Prefer assert_not_same over refute_same.
    refute_same a, b
    ^^^^^^^^^^^

2760 files inspected, 5 offenses detected, 5 offenses corrected
```
dylnclrk added a commit to dylnclrk/c5-conventions that referenced this issue Apr 30, 2019
Performance cops were moved out in to their own gem in Rubocop 0.68[1], which
means that referring to them logs a warning like:

`Warning: unrecognized cop Performance/Casecmp found in …`

Remove the reference to the performance cop to resolve this. Note that it may
negatively impact users who do not upgrade to 0.68, as this previously disabled
Performace cop will now be enabled until they upgrade to 0.68.

[1]: rubocop/rubocop#5977
natesholland added a commit to natesholland/rails that referenced this issue May 4, 2019
Performance cops were extracted in rubocop/rubocop#5977 and @koic
updated to 0.67.2 and added the rubocop-performance gem in rails#35989.

This bumps rubocop to 0.68.1 to be on the latest version. Two changes
were made in regards to the bump of the version:
* The first was that Layout/FirstParameterIndentation was renamed to
Layout/IndentFirstArgument. These changes can be seen in
rubocop/rubocop#6982 and rubocop/rubocop#6987
* The second was that a new instance of the Style/HashSyntax cop was
found. I updated that instance to follow the new Ruby >= 1.9 syntax.
benthorner pushed a commit to alphagov/govuk-lint that referenced this issue May 8, 2019
These cops are no longer available in core rubocop.

rubocop/rubocop#5977
benthorner pushed a commit to alphagov/govuk-lint that referenced this issue May 8, 2019
These cops are no longer available in core rubocop.

rubocop/rubocop#5977
osis added a commit to pivotal/LicenseFinder that referenced this issue May 8, 2019
This should have the same result. The extra gem is because this
rubocop-performance functionality was extracted from the main gem and
needed to be added back in. More info is in rubocop/rubocop#5977
osis added a commit to pivotal/LicenseFinder that referenced this issue May 8, 2019
This should have the same result. The extra gem is because this
rubocop-performance functionality was extracted from the main gem and
needed to be added back in. More info is in rubocop/rubocop#5977
osis added a commit to pivotal/LicenseFinder that referenced this issue May 14, 2019
This should have the same result. The extra gem is because this
rubocop-performance functionality was extracted from the main gem and
needed to be added back in. More info is in rubocop/rubocop#5977
ta1kt0me added a commit to grooves/forkwell_cop that referenced this issue May 31, 2019
@pirj
Copy link
Member

pirj commented Nov 6, 2020

I've just realised that you used cherry-pick to keep the history in the extracted extension.
If you happen to face a similar task in the future, I highly recommend using git-filter-repo (but be careful, it's destructive to an extent where reflog doesn't help).

@marcandre
Copy link
Contributor

👍 That's what I used to extract rubocop-ast

@bbatsov
Copy link
Collaborator Author

bbatsov commented Nov 7, 2020

@pirj Looks pretty cool! Thanks for sharing it!

@koic
Copy link
Member

koic commented Nov 7, 2020

@pirj 👍 I can't remember what was wrong, I remember giving up on git filter-branch because something couldn't be achieved at that time. And I didn't know "git-filter-repo", it could have been easier with it! 😅 Great thank you for letting me know.

renawatson68 added a commit to renawatson68/performance-develop-rubyonrails that referenced this issue Sep 23, 2022
Follow up of rubocop/rubocop#5977 (comment).

This PR prepares for release 1.0.0.
richardstewart0213 added a commit to richardstewart0213/performance-build-Performance-optimization-analysis- that referenced this issue Nov 4, 2022
Follow up of rubocop/rubocop#5977 (comment).

This PR prepares for release 1.0.0.
MarttiCheng added a commit to MarttiCheng/Rubocop-Performance that referenced this issue Sep 28, 2023
Follow up of rubocop/rubocop#5977 (comment).

This PR prepares for release 1.0.0.
SerhiiMisiura added a commit to SerhiiMisiura/Rubocop-Performance that referenced this issue Oct 5, 2023
Follow up of rubocop/rubocop#5977 (comment).

This PR prepares for release 1.0.0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted high priority A ticket considered important by RuboCop's Core Team refactoring
Projects
None yet
Development

No branches or pull requests

8 participants