Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

trailing commas in multi-line method arguments #113

Open
alanhamlett opened this issue Apr 27, 2017 · 7 comments
Open

trailing commas in multi-line method arguments #113

alanhamlett opened this issue Apr 27, 2017 · 7 comments

Comments

@alanhamlett
Copy link

alanhamlett commented Apr 27, 2017

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
)
@robotpistol
Copy link
Contributor

robotpistol commented Apr 27, 2017

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
)

@romanfuchs
Copy link

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.

@ljharb
Copy link
Contributor

ljharb commented Apr 27, 2017

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.

@alanhamlett
Copy link
Author

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

@robotpistol
Copy link
Contributor

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.

@ljharb
Copy link
Contributor

ljharb commented Apr 27, 2017

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.

@BMorearty
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants