airbnb/ruby

trailing commas in multi-line method arguments

alanhamlett opened this issue · 7 comments

Which is the preferred style?

# 1
return I18n.t('Guest Profile Page: A note for the Host saying the potential Guest has '\
              'not cancelled a reservation since joining Airbnb.',
              :default => '%{guest_name} hasn’t canceled a reservation since joining '\
                          'Airbnb',
              :guest_name => @user.smart_name)

or

# 2
return I18n.t('Guest Profile Page: A note for the Host saying the potential Guest has '\
              'not cancelled a reservation since joining Airbnb.',
              :default => '%{guest_name} hasn’t canceled a reservation since joining '\
                          'Airbnb',
              :guest_name => @user.smart_name,
)

or

# 3
return I18n.t(
  'Guest Profile Page: A note for the Host saying the potential Guest has '\
  'not cancelled a reservation since joining Airbnb.',
  :default => '%{guest_name} hasn’t canceled a reservation since joining '\
              'Airbnb',
  :guest_name => @user.smart_name,
)

or

# 4
return I18n.t(
  'Guest Profile Page: A note for the Host saying the potential Guest has '\
  'not cancelled a reservation since joining Airbnb.',
  :default => '%{guest_name} hasn’t canceled a reservation since joining '\
              'Airbnb',
  :guest_name => @user.smart_name
)

We currently don't have a rule for trailing commas in multi-line-method calls.

# good
return I18n.t('Guest Profile Page: A note for the Host saying the potential Guest has '\
                'not cancelled a reservation since joining Airbnb.',
              :default => '%{guest_name} hasn’t canceled a reservation since joining '\
                          'Airbnb',
              :guest_name => @user.smart_name)

# bad
return I18n.t('Guest Profile Page: A note for the Host saying the potential Guest has '\
                'not cancelled a reservation since joining Airbnb.',
              :default => '%{guest_name} hasn’t canceled a reservation since joining '\
                          'Airbnb',
              :guest_name => @user.smart_name,
)

# good 
return I18n.t(
  'Guest Profile Page: A note for the Host saying the potential Guest has '\
    'not cancelled a reservation since joining Airbnb.',
  :default => '%{guest_name} hasn’t canceled a reservation since joining '\
              'Airbnb',
  :guest_name => @user.smart_name,
)

# Also good
return I18n.t(
  'Guest Profile Page: A note for the Host saying the potential Guest has '\
    'not cancelled a reservation since joining Airbnb.',
  :default => '%{guest_name} hasn’t canceled a reservation since joining '\
              'Airbnb',
  :guest_name => @user.smart_name
)

Regarding the first argument, I think it's fine to not enforce one style here. It depends, e.g.

foo(argument1,
    argument2)

looks totally fine, but if it's more like

really_long_name_including_multiple_namespaces(
  argument1,
  argument2
)

then that form is probably better.

I think the rationale that should apply is, when adding a new "last" item to a list (where the current last item can have an optional trailing comma), use whichever form results in the smallest git diff.

In @romanfuchs' examples, the former would not need one; the latter would.

If this rationale makes sense to others, then I think that we should mandate it.

I like the smallest diff rule, and personally prefer 1 and 3 over the other examples.

One thing to keep in mind is that in method definitions, a trailing comma is invalid ruby syntax.

Overall I don't have a strong preference on the trailing comma in method calls though.

Prior to trailing function commas being valid JS, the JS style guide's approach was simply "everywhere a trailing comma is allowed, where adding it keeps diffs small - use one" - using the same rationale here would cover invalid syntax.

Using a trailing comma without parentheses has to be forbidden, though. It will (and has) cause errors.