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

Remove RSpec/Capybara/FeatureMethods #1876

Merged
merged 1 commit into from
May 19, 2024
Merged

Conversation

ydah
Copy link
Member

@ydah ydah commented May 3, 2024

Fix: #1852


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).

If you have modified an existing cop's configuration options:

  • Set VersionChanged: "<<next>>" in config/default.yml.

@ydah ydah requested a review from a team as a code owner May 3, 2024 09:38
config/obsoletion.yml Outdated Show resolved Hide resolved
@ydah ydah force-pushed the remove-feature-methods branch 2 times, most recently from ca1b356 to 6ff8584 Compare May 3, 2024 10:11
Copy link
Collaborator

@bquorning bquorning left a comment

Choose a reason for hiding this comment

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

I am curious to hear what @pirj thinks of this PR.

@ydah ydah requested a review from pirj May 3, 2024 11:05
@ydah ydah force-pushed the remove-feature-methods branch 3 times, most recently from 12a81f9 to 0b31e59 Compare May 3, 2024 11:31
@ydah ydah added this to the 3.0 milestone May 3, 2024
@@ -298,8 +298,15 @@ RSpec/DescribedClassModuleWrapping:
RSpec/Dialect:
Description: Enforces custom RSpec dialects.
Enabled: false
PreferredMethods: {}
PreferredMethods:
Copy link
Member

Choose a reason for hiding this comment

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

It doesn’t feel right to change the config by default.
We can’t imply that those methods are always for capybara specs.
I can’t quickly find it, but I recall someone mentioned that ‘describe’ isn’t a safe correction for ‘feature’ that has a certain metadata attached to it by default.

I suggest instead adding what needs to be done to this message below:

in .rubocop.yml file, enable `RSpec/Dialect`.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree. One thing we could do it to link from the obsoletion.yml message to https://docs.rubocop.org/rubocop-rspec/cops_rspec.html#rspecdialect, and add to the documentation in

# # ...
# end
#
class Dialect < Base
extend AutoCorrector
include MethodPreference

diff --git a/lib/rubocop/cop/rspec/dialect.rb b/lib/rubocop/cop/rspec/dialect.rb
index 7140541c..95d75161 100644
--- a/lib/rubocop/cop/rspec/dialect.rb
+++ b/lib/rubocop/cop/rspec/dialect.rb
@@ -42,6 +42,19 @@ module RuboCop
       #     # ...
       #   end
       #
+      # If you were previously using the `RSpec/Capybara/FeatureMethods` cop and
+      # want to keep disabling all Capybara-specific methods that have the same
+      # native RSpec method (e.g. are just aliases), use the following config:
+      #
+      #   RSpec/Dialect:
+      #     PreferredMethods:
+      #       background: :before
+      #       scenario:   :it
+      #       xscenario:  :xit
+      #       given:      :let
+      #       given!:     :let!
+      #       feature:    :describe
+      #
       class Dialect < Base
         extend AutoCorrector
         include MethodPreference

@pirj
Copy link
Member

pirj commented May 11, 2024

Nice! A small note on config, otherwise looks good. Thank you!

I’m sorry if I’ve missed that in some other ticket/pr/discussion, is there anything that’s stopping us from releasing 3.0 with those breaking changes as our next release? I’ve seen that we have two branches now.

@bquorning
Copy link
Collaborator

I’m sorry if I’ve missed that in some other ticket/pr/discussion, is there anything that’s stopping us from releasing 3.0 with those breaking changes as our next release? I’ve seen that we have two branches now.

No, I don’t think there’s a whole lot more to be done before releasing a v3.0 ✨ I started a discussion at #1880.

@ydah ydah force-pushed the remove-feature-methods branch 3 times, most recently from 18e76cd to 7e64055 Compare May 19, 2024 12:13
@bquorning
Copy link
Collaborator

@ydah Could you please rebase once again?

Related: Should I enable the “Always suggest updating pull request branches” feature for the repo?

Whenever there are new changes available in the base branch, present an “update branch” option in the pull request.

That would mean other maintainers could easily rebase a PR (or merge origin/master in), but perhaps at the cost of (many) more commits, and a more confusing workflow for the contributor whose local branch no longer matches the remote.

@ydah
Copy link
Member Author

ydah commented May 19, 2024

@bquorning I did it!

That would mean other maintainers could easily rebase a PR (or merge origin/master in), but perhaps at the cost of (many) more commits, and a more confusing workflow for the contributor whose local branch no longer matches the remote.

I think it's a very good point of view. IMO, but we don't want to rush the merge. And confuse PR authors. So I think it's fine the way it is.

@ydah ydah requested a review from bquorning May 19, 2024 15:29
config/obsoletion.yml Outdated Show resolved Hide resolved
@ydah ydah requested a review from bquorning May 19, 2024 15:34
Copy link
Collaborator

@bquorning bquorning left a comment

Choose a reason for hiding this comment

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

Perfect ❤️

@bquorning bquorning requested a review from pirj May 19, 2024 15:47
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.

Incredible! Thank you!

One last smoke check - does rubocop-capybara’s spec suite still pass with this?

@bquorning
Copy link
Collaborator

bquorning commented May 19, 2024

does rubocop-capybara’s spec suite still pass with this?

I tried it locally, and the specs and most of the rubocop check pass.

@bquorning
Copy link
Collaborator

I ran rubocop with this branch of rubocop-rspec on a rather large project at work, and the only unexpected issue I encountered was the problem with rubocop-rspec_rails described in #1884 (which is easily fixed after our next release). This PR is ready to merge!

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.

Remove RSpec/Capybara/FeatureMethods
3 participants