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

Allow trailing commas in multi-line [] and {}. #28

Closed
wants to merge 1 commit into from

Conversation

nixpulvis
Copy link

Having trailing commas in multi-line arrays and hashs makes diffs only
touch the lines that are related, and makes editing in general much more
consistent. We should still not use trailing commas in single line
literals, even if rubocop now allows it.

This rule in Rubocop makes very little sense, and is mostly based on
being consistent with [1,2,3] not being written [1,2,3,]. AFAIK, there
is no way to tell Rubocop to allow trailing commas in multi-line
literals, without also enabling it for single line literals. I'm pretty
sure there is an issue open about this in the rubocop repo on GH, but it
was clear most people with power didn't care, or want to change it.

Having trailing commas in multi-line arrays and hashs makes diffs only
touch the lines that are related, and makes editing in general much more
consistent. We should still not use trailing commas in single line
literals, even if rubocop now allows it.

This rule in Rubocop makes very little sense, and is mostly based on
being consistent with [1,2,3] not being written [1,2,3,]. AFAIK, there
is no way to tell Rubocop to allow trailing commas in multi-line
literals, without also enabling it for single line literals. I'm pretty
sure there is an issue open about this in the rubocop repo on GH, but it
was clear most people with power didn't care, or want to change it.
@nixpulvis
Copy link
Author

@kelvinma do you have a reason you don't like this?

@kelvinma
Copy link
Contributor

Comma implies that there is something following.

@nixpulvis
Copy link
Author

nixpulvis commented Jun 27, 2018

While I agree with that, I personally feel like the practical benefit of using them outweighs the natural language grammar. It's nice to have every line be consistent.

[
  1,
  2,
  3,
]

vs.

[
  1,
  2,
  3
]

Here the 3 is special, when it really shouldn't be. While for single line arrays, I couldn't agree more [1,2,3,] is horribly wrong.

@nixpulvis
Copy link
Author

I should listen to myself rubocop/ruby-style-guide#273 (comment)

@zcotter
Copy link
Contributor

zcotter commented Jun 28, 2018

Practical Comments
I really like having trailing commas in multiline lists/hashes because it prevents like 70% of merge conflicts I encounter.

That said, I think the change you made here would just disable the check that flags trailing commas, rather than enforcing them.
For the sake of consistency I think we should either:

  1. Leave the rule as is. It will not allow trailing commas and we will just continue to deal with the extra merge conflicts
  2. Find a way to configure the rule to enforce ALWAYS having trailing commas, and update our codebase to reflect that.

In a perfect world I think we would go with number 2, but that doesn't seem practical right now without the level of autoformatting we have in Elixir.

Academic Comments (Just for fun)
I also strongly disagree with Comma implies that there is something following. because language grammar should not apply here. The closing ] or } clearly indicates to the human reader that nothing follows. Trailing commas are preferred in other programming languages for good reason. I think the argument against using trailing commas in ruby to prevent merge conflicts is one of the few instances where ruby's much touted "human readability" gets in the way of its usability

I could also go all :neckbeard: and disagree with Nate's comment that Here the 3 is special, when it really shouldn't be..
If we want to get LISPy with things, we would see more clearly that the first N elements in a list (1, and 2,) and the last element (3) are actually different types:

  1. (cons 1 (cons 2 (cons 3 null)))
  2. (cons 2 (cons 3 null))
  3. (cons 3 null)

The first and second elements are represented by a cons type where the address register is a number and the decrement register is a reference to another cons pair.
But the last element is represented by a cons type where the address register is a number while the decrement register is either the null or empty list literal, depending on how things are implemented.
So you see, the 3 really is special after all.

@phereford
Copy link
Contributor

@zcotter It will be coming soon to elixir formatter. The core team is just waiting on formatter getting more mileage as it has only been in production since 1.6

elixir-lang/elixir#7689

@nixpulvis
Copy link
Author

nixpulvis commented Jun 28, 2018

TLDR: I'm happy with both, as they both have their merits. This is a case where there really isn't a correct answer.

I'm clearly torn between these two styles. I'm more than happy with either at this point. The practical pros can be seen as the following:

Trailing Commas:

  • Better diffs, avoids merge conflicts
  • Easier line operations, e.g. sorting all elements

No Trailing Commas:

  • More syntactically consistent with English
  • Easier editing in the case we want to compress an array down to a single line

@zcotter your argument about LISP is off. The fact that the underlying data structure is a recursive definition and uses a marker like null to indicate the base case is not necessarily how all list-like data works. More importantly, you're making a semantic argument, whereas I'm making a syntactic one. If the goal of typing something like (weird-list 1 2 3 null) is to make it clear to the reader that there is in fact a null object at the end, then it's a good way to write it, whereas, if the goal is to show that there are the elements 1, 2, 3, then I'd argue it's better to just write (list 1 2 3). :neckbeard: 🏴

P.S. I think my opinion has changed a bit on this since 2014 because I've since spent a lot of time writing macros in Racket and Rust. When you think about macros, you start thinking about sytactic consistency over semantic correctness in many cases.

@nixpulvis
Copy link
Author

nixpulvis commented Jul 3, 2018

Since conversation has fizzled, I'm closing this issue. Current style stands. Feel free to reopen if you'd like to push for a change harder. I'm easily swayed.

@nixpulvis nixpulvis closed this Jul 3, 2018
@nixpulvis nixpulvis deleted the nl-trailing-commas branch November 2, 2018 15:28
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

Successfully merging this pull request may close these issues.

4 participants