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/EmptyClass cop #9001

Merged
merged 1 commit into from Nov 8, 2020
Merged

Conversation

fatkodima
Copy link
Contributor

This cop checks for metaclasses without a body.

  # bad
  class Foo
    class << self
    end
  end

  class << obj
  end

  # good
  class Foo
    class << self
      attr_reader :bar
    end
  end

  class << obj
    attr_reader :bar
  end

@bbatsov
Copy link
Collaborator

bbatsov commented Nov 6, 2020

I'm wondering if this shouldn't be part of a more generic EmptyClass cop which can check for empty metaclass and an empty class that doesn't inherit from anything.

@fatkodima
Copy link
Contributor Author

Ha, I was 100% sure that there is already a cop for empty classes 😄
Then it should be also extended for modules, I think?

Empty modules and metaclasses are always useless, while empty classes are sometimes used and useful (for example, empty classes for some results class ResultSuccess; end, etc).

Maybe there is a better name for the cop then, if we will combine checks for classes, modules and metaclasses?

@marcandre
Copy link
Contributor

marcandre commented Nov 6, 2020

Empty modules and metaclasses are always useless,

Metaclasses: yes. Modules: not necessarily:

# main.rb:
module MyNameSpace
end

require_relative 'foo'
require_relative 'bar'
# ...

# foo.rb:
class MyNameSpace::Foo
end

# same with bar

@bbatsov
Copy link
Collaborator

bbatsov commented Nov 6, 2020

Still, I've never seen a namespace module created empty, but I guess someone might be using them in this way. We can always have a flag for this.

@marcandre
Copy link
Contributor

Still, I've never seen a namespace module created empty

Here's one I wrote myself.

I have an issue with cops that flag code that is either valid or innocuous, especially when rare. The extremely negative feelings it might generate can completely outweigh the (potentially negligible) gain. Remember, you've never seen an empty module, so clearly it's not a common mistake that gains much from being flagged and corrected.

@fatkodima
Copy link
Contributor Author

Modules: not necessarily:

Ah, forgot about this case. I have seen a couple of examples like this.

Thanks for the link to keynote, haven't seen that. It was fun 😄 Personally, I find Lint and spacing-like cops the most useful for everyone. Style, Naming, etc cops can not satisfy everyone, so projects configure them for their specific needs.

The cops I absolutely hate, when enabled, are Style/GuardClause and much-much less than previous -Style/IfUnlessModifier.

The first of them leads to code like

return ... if ...
return ... unless ...
3 more times return ...
do_something

Even, forcing me to write something like

def method
  return unless condition

  do_something
end

drives me mad a little.

@fatkodima
Copy link
Contributor Author

Changed the cop's name to EmptyClass. And now this cop checks for classes in addition to metaclasses. Decided against checking for empty modules, at least for now.

@fatkodima fatkodima changed the title Add new Lint/EmptyMetaclass cop Add new Lint/EmptyClass cop Nov 7, 2020
@bbatsov
Copy link
Collaborator

bbatsov commented Nov 8, 2020

Simple is good. :-) Thanks!

@bbatsov bbatsov merged commit c9f0389 into rubocop:master Nov 8, 2020
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

3 participants