Avoid warnings about redefined methods in EnforceSuperclass module #1466
Conversation
|
I'm not very sure that this is actually a good way to fix this issue, though it might be. But it feels sort of ugly/hacky to me. It's extremely possible that there is some more elegant way to fix the issue. I don't even really know specifically what caused this issue to emerge in version 2.30.0, which definitely limits my ability to conceive of a more elegant fix. If nothing else, I want to just submit this PR as a proof of concept / to illustrate the issue and a possible solution. But I'd be very comfortable with this PR being closed, if there's some better way to fix the issue. |
|
I didn't add any tests, because I'm not really sure that we can easily test this? This repo already has RSpec configured with |
| require 'rack/utils' | ||
| require 'active_support/inflector' | ||
| String.remove_method(:blank?) if String.instance_method(:blank?) | ||
| require 'active_support/core_ext/object/blank' |
There was a problem hiding this comment.
(This require statement is slightly misleading. It refers to active_support/core_ext/object/blank, but it not only patches Object, but also several other classes, including String.)
| module RuboCop | ||
| module Cop | ||
| module Cop # rubocop:disable Style/Documentation | ||
| remove_const(:EnforceSuperclass) if defined?(EnforceSuperclass) |
There was a problem hiding this comment.
There are three warnings printed about methods of EnforceSuperclass being redefined. Rather than undefining each of those three methods individually, here I just undefine the whole EnforceSuperclass module (if it is already defined). We will then (re-)define it just below.
|
|
||
| module RuboCop | ||
| module Cop | ||
| module Cop # rubocop:disable Style/Documentation |
There was a problem hiding this comment.
Because this RuboCop::Cop module is no longer purely just a wrapper/namespace, since below we are adding remove_const(:EnforceSuperclass) if defined?(EnforceSuperclass) to it, RuboCop now expects a documentation comment for it. But I don't think that we really want to document this module here, so I am disabling the cop here.
|
These warnings definitly have been a thing for a while already, I'm not sure why you only got them now. Tests here don't catch it because rubocop is already loaded before rspec can do its thing with I don't think your solution is the right thing to do. It removes the warnings but you might as well temporarily set I haven't looked into the |
|
How you handle Seems to be that that module here should have been scoped to a different namespace (but it wasn't, oh well). Can you add a comment that this module is also defined in RuboCop for backwards compatibility? |
davidrunger
changed the title
Fix #1465 This change avoids the printing of warnings about some methods (`included`, `on_class`, and `on_send`) of the `RuboCop::Cop::EnforceSuperclass` module being redefined. These warnings are printed because, in addition to the definition of that module here in `rubocop-rails`, that module is first [also defined in `rubocop`][1]. (The module has been extracted to `rubocop-rails`, but is also left in `rubocop`, for backwards compatibility.) [1]: https://github.com/rubocop/rubocop/blob/v1.73.2/lib/rubocop/cop/mixin/enforce_superclass.rb This change avoids the warnings by first undefining the `RuboCop::Cop::EnforceSuperclass` module (the one from `rubocop`) before then redefining that module here in `rubocop-rails`.
@Earlopain Sounds good. I have removed from this PR the removal of String#blank?, leaving that to be fixed by #1467.
Yes, good suggestion, thank you! I added this comment: rubocop-rails/lib/rubocop/cop/mixin/enforce_superclass.rb Lines 5 to 8 in f5c8ee8 |
|
Thanks! |
3 participants
Fix #1465
This change avoids the printing of warnings about some methods (
included,on_class, andon_send) of theRuboCop::Cop::EnforceSuperclassmodule being redefined. These warnings are printed because, in addition to the definition of that module here inrubocop-rails, that module is first also defined inrubocop. (The module has been extracted torubocop-rails, but is also left inrubocop, for backwards compatibility.)This change avoids the warnings by first undefining the
RuboCop::Cop::EnforceSuperclassmodule (the one fromrubocop) before then redefining that module here inrubocop-rails.Before submitting the PR make sure the following are checked:
[Fix #issue-number](if the related issue exists).master(if not - rebase it).bundle exec rake default. It executes all tests and runs RuboCop on its own code.{change_type}_{change_description}.mdif the new code introduces user-observable changes. See changelog entry format for details.If this is a new cop, consider making a corresponding update to the Rails Style Guide.(Not applicable.)