-
-
Notifications
You must be signed in to change notification settings - Fork 272
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 Capybara cops #1519
Extract Capybara cops #1519
Conversation
f55de6c
to
c820abd
Compare
@pirj I've created https://github.com/rubocop/rubocop-capybara repository and granting permissions to the RuboCop RSpec team. JFYI, It's probably best to have commit history. Once upon a time when I extracted Performance and Rails from the RuboCop core, I used However, there may be a better way :-) |
I agree, we should try hard to keep the commit history. @pirj When you move the code to the new repo, could you do it in another branch than master/main, and open a pull request for us to review and merge? |
Thanks a lot, @koic ! ❤️ |
c820abd
to
ab3b50f
Compare
rubocop-hq/rubocop-capybara#1 - reviews are welcome. |
@koic I'm trying to
May I kindly ask you to check permissions there? |
@koic @bbatsov May I kindly ask you to check permissions in https://github.com/rubocop/rubocop-capybara? Merry X-Mas! 🎄 |
Sorry for the late reply. I just checked and I had insufficient write permissions. So updated the permissions.
I've invited as a collaborator for RuboCop Capybara. Merry Christmas! 🎄 |
@pirj JFYI, I think @bquorning can also manage repositories and collaborators related to the RuboCop RSpec. Thank you. |
0d613c8
to
540a9ef
Compare
Looks green. Removed the hack to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be really nitpicking, the commit messages and PR description are a bit out of date after the recent publishing of the rubocop-capybara gem.
540a9ef
to
eaa2e36
Compare
@bquorning Fair enough 👍. Updated the PR description and the first commit's message. |
I really appreciate if you find some time and test it on some real project that is uses |
I'll dump this here for history. A method to keep the history for a limited number of files in the repo: git clone --no-local rubocop-rspec rubocop-capybara-hist
cd rubocop-capybara-hist
git-filter-repo $( (cat ../rubocop-capybara/rename | xargs -I_ echo '--path-rename _ ') )
git-filter-repo $( (cd ../rubocop-capybara && git ls-files) | xargs -I_ echo '--path _ ')
git remote add origin git@github.com:rubocop/rubocop-capybara.git
git push --set-upstream origin master
git checkout -b capybara-only
cd ../rubocop-capybara
cp -r * ../rubocop-capybara-hist
cp -r $( git ls-files | grep '^\.' | grep -v '\.github' ) ../rubocop-capybara-hist
cp -r .github ../rubocop-capybara-hist
cd -
git commit --all --message 'RSpec -> Capybara'
rvm use --create 3.0.4@rubocop-capybara
bundle
rake
git push origin capybara-only
hub pull-request --assign pirj
checklist:
|
Two questions:
I am sorry if these questions have already been asked and answered elsewhere. |
I ran
Now the TODO file contains both a section for both |
Is the intention to extract all 3 new gems in the same RuboCop-RSpec release? I keep going back and forth on whether that would be a good idea or not. |
1f3cc3d
to
83cdf7c
Compare
Good question! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good😀 Great work 🎉
As for the future extraction schedule, it seems less risky and better to complete the extraction of Capybara and confirm that there seems to be no problem after the release, and then proceed with the next extraction after we are confident of success.
Note that I am opening the following PR for FactoryBot, but I don't think it will be a stop for extraction, so please don't worry about it and proceed.
Even if the extraction is done first, the PR is modified for the post-extraction repository and sent again 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
@pirj Please let me have some feedback.
I get the following error. My local changes are below: % git diff
diff --git a/.rubocop.yml b/.rubocop.yml
index 9223aa07..c07d140c 100644
--- a/.rubocop.yml
+++ b/.rubocop.yml
@@ -16,6 +16,9 @@ AllCops:
- 'tmp/**/*'
- 'spec/smoke_tests/**/*.rb'
+RSpec/Capybara/SpecificActions:
+ Enabled: true
+
Layout/HashAlignment:
EnforcedHashRocketStyle:
- key
diff --git a/Gemfile b/Gemfile
index ad31d642..8e2c8cad 100644
--- a/Gemfile
+++ b/Gemfile
@@ -8,7 +8,7 @@ gem 'bump'
gem 'rack'
gem 'rake'
gem 'rspec', '~> 3.11'
-gem 'rubocop-capybara', github: 'rubocop-hq/rubocop-capybara'
+gem 'rubocop-capybara', github: 'rubocop/rubocop-capybara'
gem 'rubocop-performance', '~> 1.7'
gem 'rubocop-rake', '~> 0.6'
gem 'yard' It's a reproduction:
I'm sorry, I forgot this existed. In the current implementation, it may be simpler to use inheritance as the first your suggestion. As an example, I updated rubocop/rubocop#11111 to prevent breaking change. The points are below:
So, it saves the documentation generation hacks and prevents the error. |
Oops, I was using old resources. The warning displays as expected: % bundle exec rubocop --only RSpec/Capybara/SpecificActions
.rubocop.yml: RSpec/Capybara/SpecificActions has the wrong namespace - should be Capybara
--only option: RSpec/Capybara/SpecificActions has the wrong namespace - should be Capybara
(snip) Please don’t worry about it. 🙇 |
Yes, as I understand this extraction, we require our users to change any explicit |
Not really. We just warn them that they may want to change it. |
@pirj If you consider this PR done, you should feel free to merge at will. We can then release this as v2.18.0 without any other changes. Or would you prefer a v2.18.0.pre? |
`load_file` throws obsoletion errors
...and soft-aliasing the old one with a constant pointing to a class
83cdf7c
to
cc98af0
Compare
Good question. Does it require a |
Yea, exactly that. But perhaps it’s not necessary? By the way, there were a few patches merged into master yesterday. I’ll see if I can release them separately from this change even though they’re both merged. |
Sorry I've missed your plan for release and merged some patches |
No worries. It wasn’t well communicated to start with. 😅 |
It seems to me this should also go in rubocop-capybara, since it concerns the Capybara DSL. I'll be happy to lift and shift this unless there are any objections. Maybe @pirj had a reason for omitting it? |
@Drenmi It’s intentional. Even though it would only detect Captbara aliases, it is specific to RSpec aliases. It might become a more generic cop, covering more aliases to make specs less magic-y, and more RSpec’y, like e.g. https://actionpolicy.evilmartians.io/#/./testing?id=rspec-dsl |
#1440
Extracted to https://github.com/rubocop/rubocop-capybara
To test that cop renaming isn't a breaking change, add some Capybara cop configuration to some other project, and:
TODO
rubocop-capybara
toGemfile
Before submitting the PR make sure the following are checked:
master
(if not - rebase it).CHANGELOG.md
if the new code introduces user-observable changes.bundle exec rake
) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).