TrailingComma breaks on version 0.23.0

[mockdeep]

With TrailingComma set to EnforcedStyleForMultiline: comma, on previous versions this worked for us but was broken as of version 0.23.0:

attr_accessible(                                                              
  :first_name, :last_name, :email, :status, :type, :position, :full_name,        
  :school_name, :password, :password_confirmation, :phone, :twitter,          
  :outfacing_email,                                                           
)

Here's the message:

app/models/user.rb:37:21: C: Style/TrailingComma: Avoid comma after the last parameter of a method call.
    :outfacing_email,

[jonas054]

The problem is that we've changed the definition of "multiline" in this cop. Here it means one value per line, so that's the only situation in which it is okay to have a trailing comma. The point of the trailing comma is to make it easier to add and delete values in the list, and to make the impact on line based diff smaller. You don't really get those benefits if you put several values on the same line.

[mockdeep]

I think you get a pretty similar diff benefit whether you have multiple values on a line or just one. In the example above, if I had just added :outfacing_email I would have had to add the comma to the previous line in order to add it on the next line, which would have polluted the diff.

[bbatsov]

For me the value of trailing commas has always been the ability to move lines up and down and that's not something I'm ever doing with multi-element lines. The current behavior is the one that was originally intended, but wasn't originally implemented.

Perhaps this can be made configurable, but I think people are overplaying the value of diff output. There's a lengthy discussion here.

[mockdeep]

A configuration for it would be nice. I don't know about overplaying the diff issue. It's not a huge deal, but it's the one logical argument I've seen. Everything else seems to come down to subjective taste. For my own part, if I can make a slight adjustment in style to reduce the overhead of reviewing and maintaining code, I'm happy to do that, even if the improvement is only incremental.

[jonas054]

@mockdeep The purpose of the TrailingComma cop is to disallow trailing commas. It's possible, with the configuration option you used, to get it to make a consession for the case when trailing commas are useful. But it's only the maximum usefulness case where they allow you to add values anywhere in the list or move values around with small impact of diff. Your in-between case that only makes adding at the end easier is something that I personally don't want to support.

But the cop in its current state doesn't explain this distinction, so that's something I want to fix. I'll change the message and add a comment in default.yml.

[mockdeep]

@jonas054 Bummer. I guess we can find another way to do it, though it seems like it creates a bit of an inconsistency. We seldom need to rearrange elements or insert them in a particular position, unless maybe we decide to sort them. We often need to insert new elements at the end of the collection, though, regardless of where it's wrapped.

[jonas054]

@mockdeep If I understand you correctly, that means you end up with something like

attr_accessible(                                                              
  :first_name, :last_name, :email, :status, :type, :position, :full_name,        
  :school_name, :password, :password_confirmation, :phone, :twitter,          
  :outfacing_email,
  :date_of_birth,
  :favorite_food,
)

after a few additions. That is inconsistent to me, and I stand by my previous assertion that I don't want RuboCop to encourage it.

[mockdeep]

@jonas054 Rather, you end up with something like:

attr_accessible(                                                              
  :first_name, :last_name, :email, :status, :type, :position, :full_name,        
  :school_name, :password, :password_confirmation, :phone, :twitter,          
  :outfacing_email, :date_of_birth, :favorite_food,
)

[mockdeep]

We wrap it at 80 characters.

[mockdeep]

@jonas054 What makes it inconsistent for me is that instead of just saying "always add a trailing comma for multi-line collections", now we need to say "always add a trailing comma for multi-line collections, except for when you have more than one item per line". Here's a somewhat contrived example, but say I have a collection that's like:

every_word_i_know = [
  :rectangle,
  :america,
  :megaphone,
  :monday,
  :butthole,
]

And I decide, for some ungodly reason that I want to have 2 words per line to condense it down some, I suddenly have to remove the trailing comma:

every_word_i_know = [
  :rectangle, :america,
  :megaphone, :monday,
  :butthole
]