Skip to content
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 new Lint/MissingSuper cop #8376

Merged
merged 1 commit into from Jul 23, 2020
Merged

Conversation

fatkodima
Copy link
Contributor

For context - rubocop/ruby-style-guide#809

  1. I'm not sure if this should go to Lint or Style department.
  2. There is already a Style/MethodMissingSuper cop, checking only method_missing callback. Should that cop be deprecated in favor of this new cop?

@@ -137,6 +137,7 @@ def external_dependency_checksum
end

def self.inherited(subclass)
super
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😊

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fatkodima Marc-André is smiling for a reason.
There's no super for a class with no parent.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I still like adding it, and I was surprise I didn't in the first place.

@marcandre
Copy link
Contributor

marcandre commented Jul 20, 2020

Great cop & PR ❤️

@fatkodima
Copy link
Contributor Author

Thanks 😊
I'm also in need of answers to my questions to finish it.

@marcandre
Copy link
Contributor

Style/MethodMissingSuper: I'm surprised it doesn't check respond_to_missing? also, and that if you have respond_to you should also have a respond_to_missing? method.

@fatkodima
Copy link
Contributor Author

and that if you have respond_to you should also have a respond_to_missing? method.

Looks like a Style/MissingRespondToMissing is for this.

@bbatsov
Copy link
Collaborator

bbatsov commented Jul 21, 2020

There is already a Style/MethodMissingSuper cop, checking only method_missing callback. Should that cop be deprecated in favor of this new cop?

That sounds reasonable.

I'm not sure if this should go to Lint or Style department.

Lint. I'm surprised we've put the other two cops until Style, but I guess that's historical - in the very beginning Style was the only department. :-)

Looks like a Style/MissingRespondToMissing is for this.

E.g. this should probably go under Lint.

@bbatsov
Copy link
Collaborator

bbatsov commented Jul 21, 2020

I'm wondering if we should expose the ability to add more stateless classes and callbacks, but probably that's an overkill at that point. Good work!

@fatkodima
Copy link
Contributor Author

There is already a Style/MethodMissingSuper cop, checking only method_missing callback. Should that cop be deprecated in favor of this new cop?

That sounds reasonable.

Implemented this.

but probably that's an overkill at that point.

Agreed.

CHANGELOG.md Show resolved Hide resolved
end

def contains_super?(node)
node.each_descendant(:super, :zsuper).any?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any is not the same thing as !.empty?. It will work the same way in most cases, however this will fail if the array contains nil.

[1] pry(main)> [nil].any?
=> false
[2] pry(main)> ![nil].empty?
=> true

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, but each_descendant(:super, :zsuper) won't return array with nils - either empty or with Node instances.

spec/rubocop/cop/lint/missing_super_spec.rb Show resolved Hide resolved
@bbatsov bbatsov merged commit 8836872 into rubocop:master Jul 23, 2020
@ruanltbg
Copy link

ruanltbg commented Aug 6, 2020

Hi @fatkodima && @marcandre.
This cop is complaining about:

  def self.included(base)
    base.before(:each) { Warden.test_mode! }
    base.after(:each) { Warden.test_reset! }
  end

I don't see any reason for me to add a super anywhere in this code. Maybe included should be excluded from this cop? What are your thoughts on this?

Full code:

module RequestSpecHelper
  include Warden::Test::Helpers

  def self.included(base)
    base.before(:each) { Warden.test_mode! }
    base.after(:each) { Warden.test_reset! }
  end

  def sign_in(resource)
    login_as(resource, scope: warden_scope(resource))
  end

  def sign_out(resource)
    logout(warden_scope(resource))
  end

  private

  def warden_scope(resource)
    resource.class.name.underscore.to_sym
  end
end

The result from rubocop:

Offenses:

spec/support/request_spec_helper.rb:4:3: W: Lint/MissingSuper: Call super to invoke callback defined in the parent class.
  def self.included(base) ...
  ^^^^^^^^^^^^^^^^^^^^^^^

187 files inspected, 1 offense detected

@marcandre
Copy link
Contributor

marcandre commented Aug 6, 2020

Upon reflection, I have to agree that the case of included and prepended callbacks is weaker than I thought, because including a module M doesn't mean one inherits from its M.included method. In the code above, it's not like the included method blocks any method of Warden::Test::Helpers from being called.

Same idea for prepended where there won't be a chain of modules extending each other...

Unless there's objection, I'd remove them.

@fatkodima
Copy link
Contributor Author

I'd also add missing method_added/_removed/_undefined, which I forgot to add.

@marcandre
Copy link
Contributor

@fatkodima oh, good catch. Want to take care of this? You seem to write up PRs faster than I can review them 😅

@fatkodima
Copy link
Contributor Author

😄
If you have time, please do. I'm working on another one right now 😄

@marcandre
Copy link
Contributor

Sure thing: #8480

marcandre added a commit to marcandre/rubocop that referenced this pull request Aug 6, 2020
@haines
Copy link

haines commented Aug 7, 2020

Would it be possible to make the list of stateless classes configurable? We have a lot of offenses for children of a couple of base classes where we know it's unnecessary to call super in initialize.

abicky added a commit to abicky/ridgepole that referenced this pull request Aug 9, 2020
This commit fixes the following error:

> Error: The `Style/MethodMissingSuper` cop has been removed since
> it has been superseded by `Lint/MissingSuper`. Please use
> `Lint/MissingSuper` instead.

cf. rubocop/rubocop#8376
@silva96
Copy link

silva96 commented Aug 12, 2020

I have a similar problem like @haines my service classes inherit but don't need to call super

rubocop/ruby-style-guide#809 (comment)

I would like to exclude those classes or any that inherit from ApplicationService.

EDIT:

fortunately all my services objects are inside app/services so I added this to rubocop.yml

Lint/MissingSuper:
  Exclude:
    - 'app/services/**/*'

rsanheim added a commit to simpledotorg/simple-server that referenced this pull request Aug 24, 2020
This is required to make the latest version of standard happy

See rubocop/ruby-style-guide#809
and rubocop/rubocop#8376 for context
rsanheim added a commit to simpledotorg/simple-server that referenced this pull request Aug 24, 2020
* Show less months in the dropdown

* A bit less top padding for charts

* tests for to_s

* Adjust month formatting to Mon-Year

* Update standard, apply fixes that can auto apply

* Ensure we call super so parent classes can initialize

This is required to make the latest version of standard happy

See rubocop/ruby-style-guide#809
and rubocop/rubocop#8376 for context

* More linting fixes

* Bump cache versions

Just incase the date formatting change happens to mess w/ our cache keys

* Fix the keys to match the new period formatting

* Updated overview card chart spacing

Co-authored-by: claudiovallejo <hola@claudiovallejo.mx>
@@ -137,6 +137,7 @@ def external_dependency_checksum
end

def self.inherited(subclass)
super
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fatkodima Marc-André is smiling for a reason.
There's no super for a class with no parent.

expect_no_offenses(<<~RUBY)
class Foo
def method_missing(*args)
super
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this calling method_missing on Object?

expect_offense(<<~RUBY)
class Foo
def self.inherited(base)
^^^^^^^^^^^^^^^^^^^^^^^^ Call `super` to invoke callback defined in the parent class.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would super refer to? Foo has Object as an implicit parent.
Why super is needed in this case?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants