Rubocop: Auto-correct empty class definitions

[smcgivern]

The Ruby style guide says:

Prefer a single-line format for class definitions with no body.

# bad
class FooError < StandardError
end

# okish
class FooError < StandardError; end

# good
FooError = Class.new(StandardError)

I'd like to have a cop that could auto-correct to the 'good' example.

We added one in GitLab, but just for error classes, because the 'bad' example is more reasonable when, for instance, you have an empty Rails controller that just uses the default methods provided.

Here's the cop we added: https://gitlab.com/gitlab-org/gitlab-ce/commit/8dd097a91530e4b047c4b391f21047c7d29d310d

This works for us, but I have no expectations that this would work for all cases 🙂

What would we need to do to get this into RuboCop itself?

[mikegee]

An "errors only" config option on this cop would be great.

[Drenmi]

Nice one, @smcgivern! 😀

RuboCop has moved from being just a one way street towards the style guide, so all cops now are generally configurable so users can choose the style they want. This is accomplished with the ConfigurableEnforcedStyle module. See for example Style/EmptyMethod.

Oh, and you can't use the lonely operator, because RuboCop supports Ruby 2.1. 🙂

[bbatsov]

What would we need to do to get this into RuboCop itself?

Start by filing a PR. :-)

On the cop specifics - I was thinking of altering the style guide advice, as many people voiced a valid complaint over the years, that the recommended approach doesn't work very well in most editors (they fail to process this as a class definition). It's also a bit harder to spot for human readers. What I'm trying to say is that this cop should ideally be configurable.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution and understanding!

[smcgivern]

I think this is still valid, but I haven't had the time or energy to work on it 😞

I'm fine if the bot closes it; if I do come back to it in future, I can always reopen.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution and understanding!

[stale[bot]]

This issues been automatically closed due to lack of activity. Feel free to re-open it if you ever come back to it.

[Drenmi]

I re-opened this because I think it's still very relevant. I have been planning to implement "half" of this cop myself. I think there are actually two different cops here, one that takes care of whether a class definition should be single-line:

# bad
class FooError < StandardError
end

# good
class FooError < StandardError; end

and one that checks that such class definitions are written with Class.new or not:

# bad
class FooError < StandardError; end

# good
FooError = Class.new(StandardError)

[denys281]

@Drenmi What should be the flow? I meant the correct is

class FooError < StandardError; end

or

FooError = Class.new(StandardError) ?

Or it should be up to developer?

Could you help with cops naming? :blush:

[Drenmi]

What should be the flow? I meant the correct is

class FooError < StandardError; end

or

FooError = Class.new(StandardError) ?

RuboCop doesn't have opinions. It should be configurable. 🙂 (See cops with ConfigurableEnforcedStyle for reference) If there is a recommendation in The Ruby Style Guide, that should be the default.

Could you help with cops naming? 😊

There is a similar cop that does the same thing: Style/EmptyMethod, so I imagine this would be Style/EmptyClass. You can also see that cop for reference on the implementation.

Another thing to consider is that we should register an offense, but probably not auto-correct if there are comments inside the empty class body, as auto-correcting would delete the comments.

[denys281]

but probably not auto-correct if there are comments inside the empty class body, as
auto-correcting would delete the comments.

Yes, it is deleting comments.

[denys281]

@Drenmi

Should it be:

      # @example EnforcedStyle: compact (default)
      #   # bad
      #   class FooError < StandardError
      #   end
      #
      #   # bad
      #   FooError = Class.new(StandardError)
      #
      #   # good
      #   class FooError < StandardError; end
      #
      # @example EnforcedStyle: compact_constant
      #   # bad
      #   class FooError < StandardError
      #   end
      #
      #   # bad
      #   class FooError < StandardError; end
      #
      #   # good
      #   FooError = Class.new(StandardError)

[Drenmi]

As per this comment I still think this should be two separate cops. Trying to squeeze them both in one will likely lead to confusing implementation and configuration. 🙂

[koic]

I agree with @mikegee.

For example, warnings are reported for default code generated by bin/rails new and bin/rails g generator commands.

I think that many cases where an empty class is a one-liner are exception classes.
So I think it would be better to provide the following option:

# @example ExceptionClassOnly: true (default)
#   # bad
#   class FooError < StandardError
#   end
#
#   # good
#   class FooError < StandardError; end
#
#   class Foo < Bar; end
#
#   class Foo < Bar
#   end
#
# @example ExceptionClassOnly: false
#   # bad
#   class FooError < StandardError
#   end
#
#   class Foo < Bar
#   end
#
#   # good
#   class FooError < StandardError; end
#
#   class Foo < Bar; end

To determine whether it is an exception class, a regular expression (ex. /.*(Error|Exception)\z/) can be used for the superclass name. So, some exception class names may contain false negatives, but I think they can cover most cases.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution and understanding!

[stale[bot]]

This issues been automatically closed due to lack of activity. Feel free to re-open it if you ever come back to it.