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 RSpec Rails cops #1830

Merged
merged 1 commit into from Mar 29, 2024
Merged

Extract RSpec Rails cops #1830

merged 1 commit into from Mar 29, 2024

Conversation

ydah
Copy link
Member

@ydah ydah commented Mar 4, 2024

Fix: #1734

TODO

Here's where to temporary extract it:.


Before submitting the PR make sure the following are checked:

  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Updated documentation.
  • Added an entry to the CHANGELOG.md if the new code introduces user-observable changes.
  • The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).

@ydah ydah requested a review from a team as a code owner March 4, 2024 18:29
@ydah ydah mentioned this pull request Mar 4, 2024
@ydah ydah changed the title [WIP] Extract RSpec Rails cops Extract RSpec Rails cops Mar 5, 2024
@ydah
Copy link
Member Author

ydah commented Mar 5, 2024

@rubocop/rubocop-rspec We can now proceed to extraction 🎉
@bquorning Could you create https://github.com/rubocop/rubocop-rspec_rails?

@pirj
Copy link
Member

pirj commented Mar 5, 2024

Should we do rubocop-rspec_rails with underscore lije we do for rubocop-factory_bot?
As a repo name and in the code especially: we’ll use RuboCop::RSpecRails as the namespace, right?

@ydah
Copy link
Member Author

ydah commented Mar 5, 2024

Should we do rubocop-rspec_rails with underscore lije we do for rubocop-factory_bot? As a repo name and in the code especially: we’ll use RuboCop::RSpecRails as the namespace, right?

I was wondering whether it should be separated by hyphen or underscore. In factory_bot, the name of the original gem is separated by underscore, but rspec-rails is separated by hyphen.
Does the name rspec_rails sound strange? If it doesn't sound strange, I will change the name of the repository.
@rubocop/rubocop-rspec I'd like to hear your opinion.

@bquorning
Copy link
Collaborator

Based on https://guides.rubygems.org/name-your-gem/ and e.g. https://github.com/ydah/rubocop-rspec-rails/blob/207fc3483e75605b12276b90e5bb3a7ebcdc223f/lib/rubocop/cop/rspec_rails/avoid_setup_hook.rb defining RuboCop::Cop::RSpecRails and not RuboCop::Cop::RSpec::Rails, I think the gem should be named rubocop-rspec_rails.

@ydah
Copy link
Member Author

ydah commented Mar 5, 2024

Thank you. we use rubocop-rspec_rails instead of rubocop-rspec-rails.

@ydah
Copy link
Member Author

ydah commented Mar 5, 2024

I updated this and extracted PR.

@bquorning
Copy link
Collaborator

I created https://github.com/rubocop/rubocop-rspec_rails with @rubocop/rubocop-core given write-access. Ping me if anyone needs admin access.

@ydah
Copy link
Member Author

ydah commented Mar 6, 2024

@rubocop/rubocop-rspec If the modifications on the rubocop-rspec_rais side are correct, I will apply them to the https://github.com/rubocop/rubocop-rspec_rails . Please review ydah/rubocop-rspec_rails#1 .

Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

I apologise for the delay. I don’t have sufficient spare time to review this fully. I hope to do this early next week. Thank you so much for the effort you’re putting into this!

CHANGELOG.md Outdated Show resolved Hide resolved
config/obsoletion.yml Outdated Show resolved Hide resolved
Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

Looks great, thank you!
Were there any modules that are needed to to run the newly-extracted extension without rubocop-rspec? Can we remove them from rubocop-rspec? I recall we had that for capybara/factory_bot.

@pirj
Copy link
Member

pirj commented Mar 10, 2024

I can’t spot any rails-specific modules in our code at a glance, so all good.

@ydah ydah force-pushed the extract-rspec-rails branch 2 times, most recently from ba79c12 to 9649697 Compare March 28, 2024 08:55
Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

Lgtm! 😍

@pirj pirj requested review from bquorning and Darhazer March 28, 2024 09:05
@ydah
Copy link
Member Author

ydah commented Mar 28, 2024

I overlooked something serious. Gems cannot depend on each other.

❯ bundle install
Your bundle requires gems that depend on each other, creating an infinite loop. Please remove either gem 'rubocop-rspec' or gem 'rubocop-rspec_rails' and try again.

@ydah ydah requested a review from pirj March 28, 2024 09:10
@pirj
Copy link
Member

pirj commented Mar 28, 2024

I guess we’ll have to remove the dependency from rubocop-rspec_rails and make a note there that it implicitly depends on rubocop-rspec.
In 3.0, we’ll remove the dependency on rubocop-rspec_rails from rubocop-rspec, and will restore the dependency declaration in rubocop-rspec_rails.
This might be a mild surprise to those who would want just rubocop-rspec_rails, but I suppose it’s a very rare case.

@ydah
Copy link
Member Author

ydah commented Mar 28, 2024

@pirj Should rubocop-rspec_rails be a patch upgrade or a minor upgrade?

@pirj
Copy link
Member

pirj commented Mar 28, 2024

I’d yank the 2.28.0 and released a bugfix patch in 2.28.1.
@bquorning what do you think?

@bquorning
Copy link
Collaborator

bquorning commented Mar 28, 2024

I’d yank the 2.28.0 and released a bugfix patch in 2.28.1.

Yes that sounds like a good idea. And I assume nobody is using the gem yet, so the yank won’t affect users.

@ydah
Copy link
Member Author

ydah commented Mar 28, 2024

I update this PR. And All Green ✅

Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

Awesome, thank you!

This sets a major milestone, and, to me, was he last thing stopping us from releasing 3.0 of all gems and untangling them. And enabling all cops previously marked as pending.

@ydah ydah merged commit f6314a7 into master Mar 29, 2024
24 checks passed
@ydah ydah deleted the extract-rspec-rails branch March 29, 2024 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extract rspec-rails cops
3 participants