Add new Style/ClassMethodsDefinitions cop

[fatkodima]

This cop enforces how class methods are defined:

# Either
def self.class_method
  # ...
end

# or
class << self
  def class_method
    # ...
  end
end

I have checked several existing style guides, and found that:

1. shopify style guide

Use a class << self block over def self. when defining class methods, and group them together within a single block.

2. airbnb style guide

Avoid class << self except when necessary, e.g. single accessors and aliased attributes.

3. thoughtbot style guide

Use def self.method, not class << self.

So seems like people prefer to use either one of them. Personally I prefer def self.method style and noticed that rubocops codebase is also mostly based on this style. I disabled it for now, will enable if this is a useful for rubocops own codebase.

While implementing this cop (especially autocorrection), I noticed that it would be great to have the following cops: one that checks for empty class << self and another that checks for multiple class << self definitions and combines them. The second one will greatly simplify (not yet implemented) autocorrection for self_class style. Will implement both of them if sounds good.

[bbatsov]

There's also a third option - using the class name, but we might have a cop about this already, as it's definitely bad style.

So seems like people prefer to use either one of them. Personally I prefer def self.method style and noticed that rubocops codebase is also mostly based on this style. I disabled it for now, will enable if this is a useful for rubocops own codebase.

Same here. It produces the easiest to read code. I'm surprised we didn't add anything to our style guide on the topic.

While implementing this cop (especially autocorrection), I noticed that it would be great to have the following cops: one that checks for empty class << self and another that checks for multiple class << self definitions and combines them. The second one will greatly simplify (not yet implemented) autocorrection for self_class style. Will implement both of them if sounds good.

Great ideas!

The first one should probably cover empty class/module definitions in general.

[marcandre]

FWIW, I might write def self.foo if I feel lazy and think there's only going to be one, but I much prefer class << self otherwise. Reasons include:

• I'm always grepping for def foo, and when it fails I have to think about def self.foo
• I like that include / attr_reader, and al. are available to me.
• easier refactoring into a module that I extend from.

Not directly related, but I love extend self for modules of pure functions and curse when authors prefer def self.foo in a module, which makes these methods unavailable other than through Module.foo

[bbatsov]

@fatkodima ping :-)

[fatkodima]

Working on this. Will update shortly.

[fatkodima]

Enabled this cop on rubocop's codebase.

[bbatsov]

Looking at the diff it seems we shouldn't report offenses when there's a mixture of method definitions and something else in class <<. Or maybe there should be some config flag for this. I'm on the fence about this.

[fatkodima]

For def_self style, I would stick to this - https://github.com/airbnb/ruby#no-class-self and use class << self only for alias, include/extend and attr_* things. When there are private methods - do not report offenses for other methods within class << self and when only those mentioned things - report to rewrite methods with def self..

Report:

class << self
  def do_something
  end

  attr_reader :foo 
end

Do not report:

class << self
  def do_something
  end

  attr_reader :foo 

  private

  def do_something_else
  end
end

[marcandre]

Report:

class << self
  def do_something
  end

  attr_reader :foo 
end

Doesn't this contradict the rest of what you wrote?

[fatkodima]

Mmm, haven't seen any? I'm proposing a change that, when EnforcedStyle: def_self, then

class << self
  def do_something
  end

  attr_reader :foo 
end

should be rewritten as

def self.do_something
end

class << self
  attr_reader :foo 
end

And in the following case

class << self
  def do_something
  end

  attr_reader :foo 

  private

  def do_something_else
  end
end

no offense should be triggered.

Am I missing something?

[marcandre]

Am I missing something?

No, my bad, sorry for the noise 🤦‍♂️

[koic]

IMHO, I'm worried about enforcing to only either style. This is because it is doubt that decomposing the intentionally grouped class << self will get up to maintainability. If either one is the default, I think it is preferable to discuss it first in the style guide.
I think pending status is a candidate that can be enabled by default, next step. So, I think it's better to disable it by default at this point.

[marcandre]

Is there already an issue about this in the style-guide? I can't seem to find it.

[bbatsov]

Don't think so. Feel free to create one.

IMHO, I'm worried about enforcing to only either style. This is because it is doubt that decomposing the intentionally grouped class << self will get up to maintainability. If either one is the default, I think it is preferable to discuss it first in the style guide.
I think pending status is a candidate that can be enabled by default, next step. So, I think it's better to disable it by default at this point.

@koic I had suggested enabling this for RuboCop itself, not for the end users. But I agree we probably need to discuss it a bit more, as I didn't consider the cases where there something besides methods in the singleton class.

[fatkodima]

Updated:

1. made cop disabled by default
2. enabled for rubocops own codebase
3. for def_self style made to not raise an offense when class << self contains non public methods

[koic]

Thank you for your consideration. Even in RuboCop's codebase, I think it's preferable to disable it by default. This is because there are some cases where code before the change is preferable.

[bbatsov]

Fair enough, let's keep it disabled for RuboCop as well and revert the changes related to it.

[fatkodima]

Made it disabled and reverted style changes.

[bbatsov]

Thanks!