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 Rails Cops in a separate #5976

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

Extract Rails Cops in a separate #5976

bbatsov opened this issue Jun 9, 2018 · 23 comments

Comments

@bbatsov
Copy link
Collaborator

@bbatsov bbatsov commented Jun 9, 2018

Rails cops have to extracted into a standalone gem (named something like rubocop-rails 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.

Any volunteers to do this? I don't do much Rails anymore, so I'd rather not be involved in this project if possible. @koic I know you've been working a lot on the Rails cops, so perhaps you'd like to tackle the extraction?

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

@Drenmi Drenmi commented Jun 9, 2018

Unfortunately someone hogged the rubocop-rails name on RubyGems. I think the gem hosts the configuration used by the Rails team.

Loading

@bbatsov
Copy link
Collaborator Author

@bbatsov bbatsov commented Jun 9, 2018

Yeah, I noticed this, but someone on RubyKaigi mentioned the guy would be willing to give the name up. I'm fine with naming the extension rubocop-ror or whatever.

Loading

@composerinteralia
Copy link
Contributor

@composerinteralia composerinteralia commented Jun 9, 2018

I would be happy to do this, or to help @koic do it. I wrote the couple of custom cops in rails and had a lot of fun doing it. (@koic I'm guessing we take those custom cops out of rails once the next rubocop version gets released. I would be happy to make that PR and to update rubocop-rails
as well unless you were planning to do it.)

Loading

@Drenmi
Copy link
Collaborator

@Drenmi Drenmi commented Jun 9, 2018

@koic Maybe you can reach out to toshimaru about the name? His e-mail is registered in RubyGems.

Loading

@koic
Copy link
Member

@koic koic commented Jun 9, 2018

@bbatsov Yes. I will work on this issue. I'm thinking about planning now.

The following are issues and solutions for realizing this plan. There are still some issues to consider 🤔
Please let me know if you have any opinion.

RuboCop Rails Gem's name

As discussed above in this Issue, rubocop-rails gem already exists.
https://rubygems.org/gems/rubocop-rails

@Drenmi I'm asking @toshimaru whether he can transfer the gem name of rubocop-rails to the RuboCop team.

Even if toshimaru accepts the transfer gem name ownership, it seems necessary to announce to the current rubocop-rails gems (toshimaru/rubocop-rails) users, so we will need to talk to toshimaru about it.

@toshimaru Sorry to ask this of you, would you transfer rubocop-rails gem name to the RuboCop team, is it possible? I think that for the Rails department managed by RuboCop HQ, the name RuboCop Rails (rubocop-rails) is user-friendly as a convention.

Transition from RuboCop Core to RuboCop Rails

I'm thinking about the incompatibility impact on Rails department users.

Rails department users suddenly become surprised if Rails department cops are absent from RuboCop Core. I think that it is better to prepare the transition period.

So I'm considering the following transition procedure.

First, gemify RuboCop Rails and cut out.

  • Copy the Rails department from RuboCop Core repo to RuboCop Rails repo
  • Release RuboCop Rails gem (before this there is document description and some work 😉)

Prepare the following warning for RuboCop Core.

For example, the following warning is considered.

DEPRECATION WARNING: Rails department is separated and will be removed in
RuboCop 0.y.z. Please use rubocop-rails gem.

Finally, remove the Rails department from RuboCop Core. We will need to consider when removting it.

However, this procedure temporarily duplicates Rails department to RuboCop Core repo and RuboCop Rails repo. Therefore maintenance may be twice troublesome, perhaps it may be better to temporarily s.add_runtime_dependency on RuboCop Rails from RuboCop Core (validation is required).

@composerinteralia I'd like to think about a happy procedure for the Rails community as well during the transition.

About the existing issues of the Rails department

I'm thinking about the method and cost of migrating the issue to RuboCop Rails. Perhaps I will leave the existing issues to the Rails department in the RuboCop Core repo as it is, and it seems good to treat it with RuboCop Rails repo only in future issues.

Loading

@bbatsov
Copy link
Collaborator Author

@bbatsov bbatsov commented Jun 10, 2018

However, this procedure temporarily duplicates Rails department to RuboCop Core repo and RuboCop Rails repo. Therefore maintenance may be twice troublesome, perhaps it may be better to temporarily s.add_runtime_dependency on RuboCop Rails from RuboCop Core (validation is required).

I think that we can probably just cut out the new gem and show some instructions about adding the new gem when people try to enable the Rails cops. Seems this will be painless and won't really created breakages for the users (they'll just end up running fewer check temporary). I don't like much the idea of having the cops at some point in both RuboCop and RuboCop Rails, because this opens up the question of do we fix bugs in both gems or just in the new one.

We should leave some stubs for the Rails cops (or just some generic fallback logic) - e.g. each cop from the Rails department that's present in .rubocop.yml just generates a message that this cop does nothing and people need to install the new gem.

I agree that we should try to make this transition smooth, but I also think we shouldn't go overboard with it, as that's going to be one of the final big changes before 1.0. I think most users would forgive us some slight inconvenience now for the sake of long term stability.

Loading

@bbatsov
Copy link
Collaborator Author

@bbatsov bbatsov commented Jun 10, 2018

I've created a repo https://github.com/rubocop-hq/rubocop-rails so you can start hacking there. We'll renaming it later if we fails to obtain the rubocop-rails name on RubyGems.

Loading

@koic
Copy link
Member

@koic koic commented Jun 10, 2018

We should leave some stubs for the Rails cops (or just some generic fallback logic) - e.g. each cop from the Rails department that's present in .rubocop.yml just generates a message that this cop does nothing and people need to install the new gem.

I see. I didn't think of it that way, and it looks good! 🌟
First we need gemifiy, so I will work on it. Thank you for letting me know.

Loading

@toshimaru
Copy link

@toshimaru toshimaru commented Jun 11, 2018

@koic @bbatsov

would you transfer rubocop-rails gem name to the RuboCop team, is it possible?

Yes, It’s okay to give up the name if rubocop team really want it!

If I gave the name, who’d be the the author of the gem??

Loading

@Drenmi
Copy link
Collaborator

@Drenmi Drenmi commented Jun 12, 2018

@toshimaru 🙇

Loading

@bbatsov
Copy link
Collaborator Author

@bbatsov bbatsov commented Jun 12, 2018

@toshimaru I guess for now you can make me and @koic the owners. My handle on RubyGems is bozhidar, not sure about @koic's.

Loading

@koic
Copy link
Member

@koic koic commented Jun 12, 2018

@toshimaru I appreciate your acceptance very much. My RubyGems account is koic_ito (koic.ito@gmail.com). Thank you!

Loading

@koic
Copy link
Member

@koic koic commented Oct 1, 2018

The current situation. Currently, RuboCop Rails and Performance repositoies are extracting from Core using cp command. So, the git history has not been extracted now. This is inconvenient for future maintenance.

I'm investigating to extract some git history with git filter-branch or something.

After finishing the investigating, I'd like to proceed with application to the RuboCop Rails and Performance repositories. (And I may propose to RuboCop Performance first).

I appreciate your patience as it might take some time.

Loading

@bbatsov
Copy link
Collaborator Author

@bbatsov bbatsov commented Oct 1, 2018

@koic Thanks for the update! I was wondering recently how are we doing on this front.

Hopefully you'll manage to find a way to preserve the history, but if you don't - we can live without it. It's not ideal, but it's not the end of the world either.

Loading

@koic
Copy link
Member

@koic koic commented Oct 26, 2018

FYI: I cherry-picked the git histories.

Loading

@bbatsov
Copy link
Collaborator Author

@bbatsov bbatsov commented Oct 26, 2018

@koic Fantastic!

So what else remains before we can cut a release of rubocop-rails?

Loading

@koic
Copy link
Member

@koic koic commented Oct 27, 2018

@bbatsov Thanks!

I think this issue is aimed at extracting Rails department code from RuboCop Core in the future.
For that reason there is work remain to extract TargetRailsVersion from RuboCop Core to rubocop-rails. So I'm trying to extract TargetRailsVersion.

Also, I think there is some documentation about Rails department to be extracted. (Detailed documentation may be good even after release.)

Loading

@koic
Copy link
Member

@koic koic commented Oct 29, 2018

@bbatsov I have a question.

Do we extract metadata (#5978) to rubocop-rails and rubocop-performance from RuboCop Core?
If we extract metadata, we need to consider release versioning of rubocop-rails and rubocop-performance.

What do you have any ideas about this?

Related issue and PR: rubocop/rubocop-rspec#699 and #6419.

Loading

@bquorning
Copy link
Contributor

@bquorning bquorning commented Oct 29, 2018

I am curious whether RuboCop-Rails and RuboCop-Performance would not be subject to the same namespace issues that RuboCop-RSpec has been seeing. RuboCop-RSpec has an RSpec/Rails/HttpStatus cop and an RSpec/FilePath cop that sometimes conflict with RuboCop’s Rails/HttpStatus and Rails/FilePath cops. See issues like rubocop/rubocop-rspec#611, #5251, and rubocop/rubocop-rspec#710.

RuboCop-RSpec seemingly still needs the RuboCop::RSpec::Inject hack, which adds to my suspicion that we are not yet entirely ready to split out the Rails and Performance cops.

Loading

@bbatsov
Copy link
Collaborator Author

@bbatsov bbatsov commented Nov 3, 2018

Do we extract metadata (#5978) to rubocop-rails and rubocop-performance from RuboCop Core?
If we extract metadata, we need to consider release versioning of rubocop-rails and rubocop-performance.

Yeah, we'll move the metadata to the gems. Each gem will have an independent release cycle, so ideally we'll have to come up with a reasonable stable extension API during the course of this extraction process. :-)

I am curious whether RuboCop-Rails and RuboCop-Performance would not be subject to the same namespace issues that RuboCop-RSpec has been seeing. RuboCop-RSpec has an RSpec/Rails/HttpStatus cop and an RSpec/FilePath cop that sometimes conflict with RuboCop’s Rails/HttpStatus and Rails/FilePath cops. See issues like rubocop/rubocop-rspec#611, #5251, and rubocop/rubocop-rspec#710.

We'll fix such issues while tackling the migration. None of them are particularly complex.

RuboCop-RSpec seemingly still needs the RuboCop::RSpec::Inject hack, which adds to my suspicion that we are not yet entirely ready to split out the Rails and Performance cops.

Well, it's widely known that RuboCop doesn't have a proper extension API right now, but the best way to devise the API (mostly a ways for extensions to registers themselves properly, without the need for custom hacks) is to actually write a few extensions that will help the API emerge and stabilize. :-)

Loading

koic added a commit to koic/rubocop-rails that referenced this issue May 24, 2019
…DME.md

This PR adds a reference to `rubocop-rails_config` gem in README.md

Fixes rubocop#56.
Follow up rubocop#57 (comment).
Follow up rubocop/rubocop#5976 (comment).
koic added a commit to koic/rubocop that referenced this issue May 30, 2019
This PR warns for `rubocop -R/--rails` option.

Here is an example using `-R/--rails` option:

```console
% cat app/models/user.rb
class User < ActiveRecord::Base
end
```

```console
% rubocop -R
`-R/--rails` option and Rails cops will be removed from RuboCop
0.72. Use the `rubocop-rails` gem instead.
https://github.com/rubocop-hq/rubocop/blob/master/manual/migrate_rails_cops.md
Inspecting 2 files
.C

Offenses:

(snip)

app/models/user.rb:1:14: C: Rails/ApplicationRecord: Models should
subclass ApplicationRecord.
class User < ActiveRecord::Base
             ^^^^^^^^^^^^^^^^^^
```

This PR provides a way to dismiss the above warning.
Specify `require: rubocop-rails` in .rubocop.yml (or command option)
instead of the `-R/-rails` option as indicated in the above URL of warning.

And this means that it will be used the same way as RuboCop RSpec and
RuboCop Performance.

This is the migration path to remove Rails cops from the RuboCop core,
Rails cops will still be disabled by default for compatibility.
koic added a commit to koic/rubocop that referenced this issue May 30, 2019
This PR warns for `rubocop -R/--rails` option.

Here is an example using `-R/--rails` option:

```console
% cat app/models/user.rb
class User < ActiveRecord::Base
end
```

```console
% rubocop -R
`-R/--rails` option and Rails cops will be removed from RuboCop
0.72. Use the `rubocop-rails` gem instead.
https://github.com/rubocop-hq/rubocop/blob/master/manual/migrate_rails_cops.md
Inspecting 2 files
.C

Offenses:

(snip)

app/models/user.rb:1:14: C: Rails/ApplicationRecord: Models should
subclass ApplicationRecord.
class User < ActiveRecord::Base
             ^^^^^^^^^^^^^^^^^^
```

This PR provides a way to dismiss the above warning.
Specify `require: rubocop-rails` in .rubocop.yml (or command option)
instead of the `-R/-rails` option as indicated in the above URL of warning.

And this means that it will be used the same way as RuboCop RSpec and
RuboCop Performance.

This is a migration path to remove Rails cops from the RuboCop core,
Rails cops will still be disabled by default for compatibility.
bbatsov added a commit that referenced this issue May 30, 2019
This PR warns for `rubocop -R/--rails` option.

Here is an example using `-R/--rails` option:

```console
% cat app/models/user.rb
class User < ActiveRecord::Base
end
```

```console
% rubocop -R
`-R/--rails` option and Rails cops will be removed from RuboCop
0.72. Use the `rubocop-rails` gem instead.
https://github.com/rubocop-hq/rubocop/blob/master/manual/migrate_rails_cops.md
Inspecting 2 files
.C

Offenses:

(snip)

app/models/user.rb:1:14: C: Rails/ApplicationRecord: Models should
subclass ApplicationRecord.
class User < ActiveRecord::Base
             ^^^^^^^^^^^^^^^^^^
```

This PR provides a way to dismiss the above warning.
Specify `require: rubocop-rails` in .rubocop.yml (or command option)
instead of the `-R/-rails` option as indicated in the above URL of warning.

And this means that it will be used the same way as RuboCop RSpec and
RuboCop Performance.

This is a migration path to remove Rails cops from the RuboCop core,
Rails cops will still be disabled by default for compatibility.
koic added a commit to koic/rubocop that referenced this issue May 31, 2019
Fixes rubocop#5976.

This PR removes Rails department from RuboCop core.

- Remove Rails department cops
- Remove `-R/--rails` option from `rubocop` command

These were transferred to RuboCop Rails gem.
https://github.com/rubocop-hq/rubocop-rails

Some internal implementations of `TargetRailsVersion` remain. The final
goal in the near future is to extract `TargetRailsVersion`
implementation to RuboCop Rails from RuboCop core.
rubocop/rubocop-rails#47

This PR aims removing Rails cops and `rubocop -R /-rails` option that
affect RuboCop 0.72.0 users.

It may be better to merge this PR when we decide the next release
version is RuboCop 0.72.
koic added a commit to koic/rubocop that referenced this issue May 31, 2019
Fixes rubocop#5976.

This PR removes Rails department from RuboCop core.

- Remove Rails department cops
- Remove `-R/--rails` option from `rubocop` command

These were transferred to RuboCop Rails gem.
https://github.com/rubocop-hq/rubocop-rails

Some internal implementations of `TargetRailsVersion` remain. The final
goal in the near future is to extract `TargetRailsVersion`
implementation to RuboCop Rails from RuboCop core.
rubocop/rubocop-rails#47

This PR aims removing Rails cops and `rubocop -R /-rails` option that
affect RuboCop 0.72.0 users.

It may be better to merge this PR when we decide the next release
version is RuboCop 0.72.
koic added a commit to koic/rubocop that referenced this issue May 31, 2019
Fixes rubocop#5976.

This PR removes Rails department from RuboCop core.

- Remove Rails department cops
- Remove `-R/--rails` option from `rubocop` command

These were transferred to RuboCop Rails gem.
https://github.com/rubocop-hq/rubocop-rails

Some internal implementations of `TargetRailsVersion` remain. The final
goal in the near future is to extract `TargetRailsVersion`
implementation to RuboCop Rails from RuboCop core.
rubocop/rubocop-rails#47

This PR aims removing Rails cops and `rubocop -R /-rails` option that
affect RuboCop 0.72.0 users.

It may be better to merge this PR when we decide the next release
version is RuboCop 0.72.
@bbatsov bbatsov closed this in #7095 Jun 2, 2019
bbatsov added a commit that referenced this issue Jun 2, 2019
Fixes #5976.

This PR removes Rails department from RuboCop core.

- Remove Rails department cops
- Remove `-R/--rails` option from `rubocop` command

These were transferred to RuboCop Rails gem.
https://github.com/rubocop-hq/rubocop-rails

Some internal implementations of `TargetRailsVersion` remain. The final
goal in the near future is to extract `TargetRailsVersion`
implementation to RuboCop Rails from RuboCop core.
rubocop/rubocop-rails#47

This PR aims removing Rails cops and `rubocop -R /-rails` option that
affect RuboCop 0.72.0 users.

It may be better to merge this PR when we decide the next release
version is RuboCop 0.72.
vzamanillo added a commit to vzamanillo/linter-rubocop that referenced this issue Jun 14, 2019
According to rubocop/rubocop#5976 Rails department was removed from Rubocop core and extracted to a plugin, anyway this option had no effect because was removed accidentaly on e9c0319
vzamanillo added a commit to vzamanillo/linter-rubocop that referenced this issue Jun 14, 2019
According to rubocop/rubocop#5976 Rails department was removed from Rubocop core and extracted to a plugin, anyway this option had no effect because was removed accidentaly on e9c0319
app2641 added a commit to grooves/forkwell_cop that referenced this issue Jun 26, 2019
Mr0grog added a commit to edgi-govdata-archiving/web-monitoring-db that referenced this issue Jul 10, 2019
In the latest rubocop, all the Rails-related cops were extracted into a new rubocop-rails gem, so this adds that gem. For more, see: rubocop/rubocop#5976
Mr0grog added a commit to edgi-govdata-archiving/web-monitoring-db that referenced this issue Jul 10, 2019
In the latest rubocop, all the Rails-related cops were extracted into a new rubocop-rails gem, so this adds that gem. For more, see: rubocop/rubocop#5976
tonyta added a commit to tonyta/adopt-a-drain that referenced this issue Jul 26, 2019
Rubocop was updated to v0.72.0 in 9efc6a0. That version extracted Rails
cops to a separate gem: rubocop rails.

  rubocop/rubocop#5976

This commit reintroduces the missing Rails cops.
ifricker added a commit to q-centrix/hound that referenced this issue Aug 6, 2019
Rubocop extracted all rails checks to separate gem
rubocop/rubocop#5976
artur-beljajev added a commit to internetee/style-guide that referenced this issue Aug 14, 2019
jasdeepgosal added a commit to charitywater/cw-style that referenced this issue Aug 26, 2019
It was extracted out: rubocop/rubocop#5976

[#166913897]
JureCindro added a commit to infinum/default_rails_template that referenced this issue Nov 12, 2019
- added rubocop-rails gem and require rubocop-rails cops
(rubocop/rubocop#5976)
- added departments for all cops
- changed word and symbol arrays cops from disabled to enforcing style
brackets, forceing us to using a consistent style across the app instead
of letting us use both.
- removed disabling of mutable constants cop, constants should be
constant 🤓
JureCindro added a commit to infinum/default_rails_template that referenced this issue Nov 12, 2019
- added rubocop-rails gem and require rubocop-rails cops
(rubocop/rubocop#5976)
- added departments for all cops
- changed word and symbol arrays cops from disabled to enforcing style
brackets, forceing us to using a consistent style across the app instead
of letting us use both.
- removed disabling of mutable constants cop, constants should be
constant 🤓
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

7 participants