-
-
Notifications
You must be signed in to change notification settings - Fork 284
Add v2 update docs #1013
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
Add v2 update docs #1013
Conversation
|
@sl4vr appreciate if you could take a look. |
ba1735a to
625a296
Compare
Darhazer
left a comment
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.
Great job! 🙇
|
Extended the guide in regards to this change rubocop/rubocop#8490 |
edef2bb to
d4a8412
Compare
d4a8412 to
494ee04
Compare
|
@bquorning Green. |
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.
Sorry about the many nitpick comments here.
I believe this PR should be the last one to be merged into release-2.0, as it may still change depending on the other PRs getting merged.
a07ba4c to
beb0438
Compare
|
Thanks for proofreading, @bquorning !
Agree 👍 |
d155443 to
01e4e2b
Compare
|
All review notes applied ✔️ |
cc41715 to
03e1a97
Compare
|
@bquorning Ready for the review. There are changes since the last review, the list is in TODO section of the ticket description. |
sl4vr
left a comment
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.
Just found a few inaccuracies.
|
Eagle eye, @sl4vr, thanks a lot! |
17126e2 to
5e3d7de
Compare
| - cop departments are nested for cops with a department that doesn’t match the extension name (`Capybara`, `FactoryBot`, `Rails`) | ||
| - `AllCops/RSpec/Patterns`/`AllCops/FactoryBot/Patterns` options are removed | ||
| - Calling `super` from `on_new_investigation` defined in a cop is mandatory now | ||
| - Avoid explicit definition of `cop` as `described_class.new` |
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 don’t understand this sentence. Could you explain a bit more, please?
c0bd4cb to
5a9714e
Compare
|
Suggestions applied, clarifications made, fixups pushed, later squashed. Thanks for the review, @bquorning ! |
|
|
Better viewed as https://github.com/rubocop-hq/rubocop-rspec/blob/add-v2-migration-docs/docs/modules/ROOT/pages/upgrade_to_version_2.adoc
TODO
Includeshould be used instead ofPatternsInclude's merge mode is set to override by defaultsuperif you define on_new_investigation in a coplet(:cop) { described_class.new }in custom cops' specs, as this will break specs (config not loaded)Before submitting the PR make sure the following are checked:
master(if not - rebase it).CHANGELOG.mdif 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).