rubocop / rubocop-rspec

Code style checking for RSpec files.
https://docs.rubocop.org/rubocop-rspec
MIT License
792 stars 272 forks source link

Better assist rspec rails cop users when migrating to 3 #1931

Closed jeppester closed 3 days ago

jeppester commented 1 week ago

Related issue: https://github.com/rubocop/rubocop-rspec/issues/1928

Previously you would first get:

Error: The `RSpec/Rails/InferredSpecType` cop has been moved to `RSpecRails/InferredSpecType`.
(obsolete configuration found in .rubocop.yml, please update it)

Then after renaming the cop, you would get:

Error: unrecognized cop or department RSpecRails/InferredSpecType found in .rubocop.yml
Did you mean `RSpec/RepeatedDescription`?

Which would be a dead end.

This is my attempt to lead users in a better direction.

I'm unsure about how to test this. Are there tests for obsoletion texts? I'm also unsure if I need to check the documentation checkboxes for this?


Before submitting the PR make sure the following are checked:

jeppester commented 1 week ago

We should probably use the extracted key instead, extracted of removed, e.g.

   extracted:
     RSpec/Rails/*: rubocop-rspec_rails

Nice, I was not aware of this feature, I just reused what was already in the config file :+1:

I don’t see that RuboCop tests the extracted feature with nested namespaces, so we should at least to manual testing to ensure it actually works.

I'll have a look

In v3.0.0 we also extracted rubocop-factory_bot and rubocop-capybara, so we should fix the config for those cops as well.

Makes sense. I took those from https://github.com/rubocop/rubocop-rspec/pull/1830, but it would obviously make sense to include the others.

Let me know if this is too much scope creep; I’m happy to fix it myself as well 😄

I'll give it a shot, then hand it over if it grows too much in complexity. 😄

jeppester commented 1 week ago

I've changed it as suggested.

The extra layer of nesting seems to be working fine.

  1. Before adding rubocop-rspec_rails I get:

    Error: `RSpec/Rails/*` has been extracted to the `rubocop-rspec_rails` gem.
    (obsolete configuration found in .rubocop.yml, please update it)
  2. After adding rubocop-rspec_rails to require, rubocop finishes successfully, but I get a warning:

    .rubocop.yml: RSpec/Rails/InferredSpecType has the wrong namespace - replace it with RSpecRails/InferredSpecType
  3. After updating the config file, the warning goes away.

Will this warning trigger for all the affected cops? And is it okay that it's only a warning? I'm thinking if it would make sense to add the renamed config that this PR removes from rubocop-rspec to each of the new gems respectively, or if that would be unnecessary?

bquorning commented 1 week ago

I'm thinking if it would make sense to add the renamed config that this PR removes from rubocop-rspec to each of the new gems respectively, or if that would be unnecessary?

@ydah what you think about this suggestion?

pirj commented 1 week ago

I can't push to your master @jeppester , can you please add the changelog (please feel free to reword):

diff --git CHANGELOG.md CHANGELOG.md
index f35c9f36..a932dab9 100644
--- CHANGELOG.md
+++ CHANGELOG.md
@@ -4,6 +4,7 @@

 - Fix wrong autocorrect for `RSpec/ScatteredSetup` when hook contains heredoc. ([@earlopain])
 - Fix false negative for `RSpec/PredicateMatcher` when expectation contains custom failure message. ([@earlopain])
+- Facilitate the 3.0 upgrade flow with proper extracted cop messages. ([@jeppester])

 ## 3.0.1 (2024-06-11)

@@ -936,6 +937,7 @@ Compatibility release so users can upgrade RuboCop to 0.51.0. No new features.
 [@jaredmoody]: https://github.com/jaredmoody
 [@jdufresne]: https://github.com/jdufresne
 [@jeffreyc]: https://github.com/jeffreyc
+[@jeppester]: https://github.com/jeppester
 [@jessieay]: https://github.com/jessieay
 [@jfragoulis]: https://github.com/jfragoulis
 [@johnny-miyake]: https://github.com/johnny-miyake
ydah commented 1 week ago

I'm thinking if it would make sense to add the renamed config that this PR removes from rubocop-rspec to each of the new gems respectively, or if that would be unnecessary?

@ydah what you think about this suggestion?

I would like to know how the output would look like if I add that.

ydah commented 1 week ago

Perhaps the following changes should be implemented in rubocop-capybara, rubocop-factory_bot and rubocop-rspec_rails.

# example of rubocop-capybara
extracted:
  RSpec/Capybara/*: ~

refs: https://github.com/rubocop/rubocop-rails/commit/ec173904751ece664f000ee8c999fdeaae5ab98c

pirj commented 1 week ago

I think that extracted made renamed entirely redundant in our case.

The output for the cases when the extracted gems aren’t bundled and are bundled, but extracted cops are still referenced by their old names are in this comment.

I’d be most interested in edge cases, like:

  1. Telling the user to also explicitly enable those new departments if they have AllCops/Enabled: false
  2. Double-check if we’ve done something more than just changed the department like RSpec/Capybara/PredicateMatcher Capybara/RSpec/PredicateMatcher (instead of just Capybara/PredicateMatcher. @ydah would you like to check how it worked for our users during the upgrade to 3.0 with renamed obsoletion and the new extracted? Like you say, we may need to add renamed in the extracted extensions’ obsoletion config.

In any case, I don’t see those as blockers, as this change makes the upgrade smoother anyway.

jeppester commented 1 week ago

I can't push to your master @jeppester , can you please add the changelog (please feel free to reword):

Done.

Let me know if there's more I need to do.

ydah commented 4 days ago

@jeppester Could you squash two commits?

jeppester commented 3 days ago

@jeppester Could you squash two commits?

Done