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
New RuboCop autocorrect #955
Conversation
CodeClimate complains about some added complexity, which should probably be addressed. But I think it’s important that we get a new release out pretty quickly, because right now we are incompatible with the most recent version of RuboCop. |
There's a lot of activity 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 for taking care of this.
I left a few comments, but I'm fine with merging as-is 🚀
@@ -301,6 +270,7 @@ def replacement_matcher(node) | |||
# # good - the above code is rewritten to it by this cop | |||
# expect(foo.something?).to be_truthy | |||
class PredicateMatcher < Cop | |||
extend AutoCorrector |
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.
Why is this needed?
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.
A cop must extend AutoCorrector
to be able to autocorrect: https://github.com/rubocop-hq/rubocop/blob/v0.87.1/lib/rubocop/cop/base.rb#L304
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.
Oh, I didn't notice this is the actual cop, as it is like 270 lines below the start of the code, though it was a helper class
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.
Still somehow confusing, as the offsense/correction is provided by the included modules. Do we need the autocorrect
method in this class?
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.
Shouldn't it be PredicateMatcher < Base
?
If is it reused, the module
should probably do it itself:
def self.included(base)
base.extend AutoCorrector
end
@Darhazer |
The autocorrection API was changed in Rubocop v0.87.0 (pull request rubocop/rubocop#7868). Here, I change the superclass of `RuboCop::Cop::RSpec::Cop` from `::RuboCop::Cop::Cop` to `::RuboCop::Cop::Base`. This has a few consequences: - Our `#message` methods get called with a different argument than before. It *can* be customized by defining a `#callback_argument` method, but I think it is clearer to avoid callbacks altogether. - Our `#autocorrect` methods don't get called anymore. Instead, we extend `Autocorrector`, and the `corrector` is being yielded when calling `#add_offense`, so the code is mostly moved in there. For some cases, this means that some code can be removed, which is nice. For some cases, it means that the methods get too long, or the code complexity gets too high, and in those cases I chose to just call out to an `#autocorrect` method anyway, but of course passing the `corrector` and any usable local variables along. This also means we bump the dependency of RuboCop quite a bit, from '>= 0.68.1' to '>= 0.87.0'.
a233e17
to
ac953d8
Compare
Wow, impressive PR 💪! |
Thanks. I hope I got everything right 😅 |
Sleek! 👏 |
The autocorrection API was changed in Rubocop v0.87.0 (pull request rubocop/rubocop#7868). This PR makes rubocop-rspec compatible with those changes.
The superclass of
RuboCop::Cop::RSpec::Cop
is changed from::RuboCop::Cop::Cop
to::RuboCop::Cop::Base
.This has a few consequences:
#message
methods get called with a different argument than before. It can be customized by defining a#callback_argument
method, but I think it is clearer to avoid callbacks altogether.#autocorrect
methods don't get called anymore. Instead, we extendAutocorrector
, and thecorrector
is being yielded when calling#add_offense
, so the code is mostly moved in there. For some cases, this means that some code can be removed, which is nice. For some cases, it means that the methods get too long, or the code complexity gets too high, and in those cases I chose to just call out to an#autocorrect
method anyway, but of course passing thecorrector
and any usable local variables along.See also https://github.com/rubocop-hq/rubocop/blob/master/docs/modules/ROOT/pages/v1_upgrade_notes.adoc for documentation of many of the changes.
We probably need some documentation changes too; I haven’t looked into that yet.
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).