Deprecate EnsureNode#body in favour of EnsureNode#branch#337
Conversation
| # @deprecated Use `EnsureNode#branch` | ||
| def body | ||
| first_caller = caller(1..1).first | ||
|
|
||
| unless DEPRECATION_WARNING_LOCATION_CACHE.include?(first_caller) | ||
| warn '`EnsureNode#body` is deprecated and will be changed in the next major version of ' \ | ||
| 'rubocop-ast. Use `EnsureNode#branch` instead to get the body of the `ensure` branch.' | ||
| warn "Called from:\n#{caller.join("\n")}\n\n" | ||
|
|
||
| DEPRECATION_WARNING_LOCATION_CACHE << first_caller | ||
| end | ||
|
|
||
| branch | ||
| end |
There was a problem hiding this comment.
I'm not sure if this is overkill?
I thought it'd be useful to output a warning given that the method will no longer work the same post #336 release. At the same time, I am going to update all the @rubocop org repos to use the new method name, so the risk is only to 3rd party extensions that might use EnsureNode#body.
I also wanted it to not spam deprecation warnings, hence the cache to make sure each warning is just output once.
There was a problem hiding this comment.
@Earlopain it looks like StrictWarnings is causing the rubocop master specs to fail because of the warning 🤦. Any ideas?
There was a problem hiding this comment.
Well, this is awkward. I didn't anticipate this interaction.
Currently there is nothing to disable this but it is easy enough to control through environment variable. It already does nothing if CI is not set but I don't think that would be appropriate here. I can make a PR tomorrow.
Rails sets STRICT_WARNINGS=1 in their CI instead of relying on something generic, once again I should just have followed what they do.
There was a problem hiding this comment.
I'm not sure if this is overkill?
I don't think so, I like it.
dvandersluis
force-pushed
the
deprecate-ensure-body
branch
from
0f8bfb4 to
a504d31
Compare
|
Thank you @Earlopain! Everything is green now. Marking this PR as ready for review. |
|
Great. Could you add a Changelog entry please 🙏 |
dvandersluis
force-pushed
the
deprecate-ensure-body
branch
from
a504d31 to
f104fb6
Compare
|
Yes! Updated now. |
|
Thanks so much ❤️ Released as 1.36.0 |
|
One consideration, I don't believe this should show the deprecation warning rightaway. There is currently no RuboCop version that will not show this warning |
3 participants
Given the upcoming method change in #336, but also the fact that the tests of that PR cannot pass as-is since rubocop is still relying on the old definition, I thought it would make sense to deprecate
EnsureNode#bodyand add#branchas an alias. Once released, I can update the uses ofEnsureNode#bodyin rubocop (and extensions) without breaking anything, which will allow the "Main Gem Specs" CI to pass.