Don't define #autocorrect in mixins

[Drenmi]

I want to argue that we shouldn't define the #autocorrect method in mixins used by cops.

What's wrong with it?

1. If I want to use a mixin in a cop that shouldn't implement auto-correct, I need to add a no-op #autocorrect definition to the cop. This is pretty icky.
2. If I want to use two mixins, both implementing #autocorrect in a cop, which method is finally defined depends on the order of the mixins. This is very fragile.
3. There might be more than one #autocorrect that makes sense for a mixin, depending on the context it is being used.
4. There is no easy way to see if, and if so which, auto-correct is defined from within a Cop body.

What should we do instead?

We already have a suitable abstraction in the callable objects that #autocorrect return. A mixin could simply make any number of correctors available for the Cop to return from it's own #autocorrect method.

Another option could be to have the mixin provide lower level methods that the cop can use to assemble it's own #autocorrect. The Parentheses mixin could provide #remove_parentheses and #add_parentheses, for example.

Which mixins are currently implementing it?

• AutocorrectAlignment
• EmptyLinesAroundBody
• FirstElementLineBreak
• MultilineLiteralBraceLayout
• Parentheses
• SpaceAfterPunctuation
• SpaceBeforePunctuation
• SpaceInside
• StringLiteralsHelp
• TrailingComma
• UnusedArgument

[bbatsov]

I agree. We should change this.

On Friday, 30 September 2016, Ted Johansson notifications@github.com
wrote:

I want to argue that we shouldn't define the #autocorrect method in
mixins used by cops.
What's wrong with it?

If I want to use a mixin in a cop that shouldn't implement
auto-correct, I need to add a no-op #autocorrect definition to the
cop. This is pretty icky.
2.

If I want to use two mixins, both implementing #autocorrect in a cop,
which method is finally defined depends on the order of the mixins. This is
very fragile.
3.

There might be more than one #autocorrect that makes sense for a
mixin, depending on the context it is being used.
4.

There is no easy way to see if, and if so which, auto-correct is
defined from within a Cop body.

What should we do instead?

We already have a suitable abstraction in the callable objects that
#autocorrect return. A mixin could simply make any number of correctors
available for the Cop to return from it's own #autocorrect method.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#3558, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAGVyuByzs6j-wdGBdU-95dGTofJ-wycks5qvSJggaJpZM4KLLE9
.

[garettarrowood]

I'm a fan of refactoring and these are great points. After working on bug reports from the new release, I'd be happy to tackle this.

[Drenmi]

@garettarrowood: I've been thinking about this quite a bit. Since the #autocorrect method just needs to return an object that responds to #call, it is simple to extract common correctors into classes, and keep them in their own directory.

[garettarrowood]

So, if I understand correctly, this would mean a new set of modules called correctors. All "corrector" methods/helpers would be moved out of mixins/* and into something like cop/correctors/space_correctors.rb. I'd be in great favor of a filename/classname convention with these so it is clear inside the cop class. This would be similar to Rails conventions for helpers, presenters, jobs, controllers, etc. filenames.

Example:

class SpaceInsideArrayLiteralBrackets < Cop
  include SurroundingSpace
  include SpaceCorrectors
  include ConfigurableEnforcedStyle

  ...
end

Is this what you had in mind?

[Drenmi]

@garettarrowood: Pretty much. Except I see no reason to mix the corrector methods into the cop. The Cop class is already pretty overloaded, and correction is a concern that can be delegated to the corrector. I.e. composition over inheritance. 🙂