-
Notifications
You must be signed in to change notification settings - Fork 0
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 #1
Conversation
0da51f0
to
0d3abcb
Compare
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.
I tried diff'ing this code against a checkout of rubocop-factory_bot and rubocop-capybara, and it looks mostly ok. There are a few differences that I’ve tried to comment on.
Co-authored-by: Benjamin Quorning <22333+bquorning@users.noreply.github.com>
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.
I updated this PR.
Co-authored-by: Phil Pirozhkov <pirj@users.noreply.github.com>
Co-authored-by: Benjamin Quorning <22333+bquorning@users.noreply.github.com>
12a8416
to
195dbcf
Compare
I updated this PR. |
@bquorning @pirj
|
lib/rubocop/rspec_rails/shared_contexts/default_rspec_language_config_context.rb
Outdated
Show resolved
Hide resolved
lib/rubocop/rspec_rails/shared_contexts/default_rspec_language_config_context.rb
Outdated
Show resolved
Hide resolved
This is really close to readiness. Just one thing to reuse the config left. Thank you for the immense effort! |
@pirj Thank you for your review! I updated this PR. |
I’m sorry for not having had the capacity to review your PR again. |
@bquorning Don't worry about it. Thank you as always 😄 ! |
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.
Thank you!
rubocop-rspec_rails.gemspec
Outdated
'rubygems_mfa_required' => 'true' | ||
} | ||
|
||
spec.add_runtime_dependency 'rubocop', '~> 1.40' | ||
spec.add_runtime_dependency 'rubocop-capybara', '~> 2.17' | ||
spec.add_runtime_dependency 'rubocop-factory_bot', '~> 2.22' | ||
spec.add_runtime_dependency 'rubocop-rspec', '~> 2.27.1' |
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.
Should we make it depend on 2.28.0 where all cops are extracted to prevent duplicates? I can’t recall how we did this in the past.
Point to github from Gemfile to ensure edge, release this gem first, then release rubocop-rspec 2.28.0, then change this gemfile to remove github source and point to rubygems.
I haven’t had my morning coffee yet, so please take this idea with a grain of salt.
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.
In the past we didn't need to worry about it, since rubocop-capybara and rubocop-factory_bot didn't rely on rubocop-rspec. Sorry. I want you to tell me, what kind of problem do I have if I don't depend on it after it's extracted?
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.
In theory, it would be possible to install rubocop-rspec_rails 2.28.0 with rubocop-rapec 2.27.1, where the extraction didn’t happen yet.
It could happen, but seems unrealistic.
Other cases would be like the one we’ve had when we wanted to yank a certain version? In some circumstances, bundler can prefer to downgrade rubocop-rspec to this combination of versions.
rubocop/rubocop-rspec#1567
And, we should lick this to <3.0 here
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.
Thank you. Indeed it is. I update this PR 1f412fd
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.
Ah, I see, the version in edge is 2.27.1, so it won’t work to point to it.
@bquorning @Darhazer do you foresee any issues with the current approach?
@bquorning @pirj @Darhazer Now that all the problems have been solved, I will proceed with the extraction if there are no other problems. |
Okay, I'm going to proceed with the extraction. |
Commands:
rename:
|
No description provided.