-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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 should be encouraged in multiline literals #273
Comments
I'm against such a change. There's not a single strong argument to support this style:
We shouldn't add superfluous characters to work around tool limitations.
There's no data to back such claim and based on my observations of Ruby projects it's unlikely that someone will gather it. The style's popular in the Python community for sure, but it's rarely used in Ruby projects. |
This is itself a non-reason. Are there any arguments against this style?
Actually, the whole point of a style guide is to work around tool limitations. If our canonical representation of code was not text, we could completely disregard style such as whitespace, hash rockets, etc. and just have editors render the code in our preferred style. Given this, I think simpler VCS diffs are totally a legit argument for this style.
Sure, I agree. Again, this is not a reason against.
You're making exactly the assertion that you challenge. There is no conclusive data either way, so this point is not valid in either direction. For me, the biggest win is the VCS thing, and I don't see why that's not a legitimate thing to consider. What do you think? |
At the risk of stating the obvious, we're talking about things like: [
534,
78,
]
{
a: 'foobar',
b: 'foobaz',
} and the question is whether to have a comma after 78 or foobaz, right? I'd be in the pro-comma camp, partly for VCS but also line-oriented commands in text editors. As for what is more common in the ruby community, I don't know about that and it could be a legitimate concern (I am, for the most part, a fan of a style guide which follows, rather than leads, existing practice). |
I got out of the habit of trailing commas when it would break my JS in IE6. I love that we now live in a world where that's not a problem. I'd be in favor of ensuring a trailing comma exists. |
I didn't see this benefit mentioned: it's not uncommon to move elements in arrays and hashes around while refactoring/changing logic--when there's a trailing comma, this movement doesn't involve an "add a comma to the former last element & remove a comma from the new last element" step. |
Commas are meant to be used as separators, using them in any other way is kind of against their semantics. I'm saying this in a general sense. Since code layout is mostly for the benefit of humans we should optimize it for human minds - personally, when I see a comma, I expect to see another item following it. Consider this:
The second expression would often be interpreted by a human mind as incomplete. @tamird Whitespace, newlines, etc serve exactly the same purpose - making it friendly for our brains. |
Commas aren't strictly seperators in ruby; the fact that [
:hello,
:world,
] is valid should prove that. |
@indspenceable To quote myself - x = 5,
y = 6
# => [5, 6] The result would likely surprise some people. :-) |
|
@bbatsov but we're not talking about the general case -- this discussion is framed as "...multiline literals". Agree? |
I'm fully in the trailing commas camp. Using them provides cleaner diffs and the easier ability to reorder elements. I agree they look bizarre in a single line literal, but they look fine to me in multiline literals which as the case discussed here. I see multiple benefits and not a single real drawback. |
ping @bbatsov can we get this change made? |
No no no, the point of a style guide is to provide guidance on how to write code when there is more than one way to write the same thing. |
Those are very similar things. The reason a syntax checker in an editor won't make this change automatically is because trailing commas or missing trailing commas are both syntactically valid ways to write the same thing. If the language were more compact then the tools could be, too. |
This rule is horrible. I'd like all the minutes back of my life that I've spent dealing with JSON enforcing this rule and half the time I rearrange a block of JSON it fails to parse and I wind up either need to whack off the trailing comma and/or track down where I copied the previous trailing comma-less entry. We are not writing literature here, we are trying to efficiently write software. This rule gets in the way. Breaks the ability to manipulate text purely as lines. And then there's always the additional churn in diffs. |
I feel very, very strongly that trailing commas are better. Cleaner diffs, consistency with other lines... |
I used to argue the same points, however I've had this conversation enough times now to realize that diffs are a really weak argument for coding a specific way. array = [1, 2, 3, 4, 5]
array = [
1,
2,
3,
4,
5
]
never = [1, 2, 3, 4, 5,] The comma is logically a separator between elements of the array/hash. Separating nothing implies subtly that there is something after the last element. |
@nixpulvis again, this discussion is about multiline literals. |
That's my point, they are conceptually the same thing. Updated for clarity. |
I am in favour of this change, but only if the final comma is replaced with U+1F4A9, otherwise known as PILE OF POO. 💩 |
@jamesotron omg, is that even possible |
@nixpulvis the diffs are a weak argument. the mistakes made when manipulating multiline structures is the stronger argument. the default that rubocop has now actively encourages syntax mistakes. |
@lamont-granquist That's not correct at all. If you're using editor integration for RuboCop (or even the basic MRI linter) you'll get immediate feedback for such trivial syntax errors. That's why I don't place much value on this argument. |
Totally agree 100% on recommending trailing commas in multi-line arrays and hashes. Here's my thought process around it.
First, the VCS argument, plus this other one quoted above, are great reasons for supporting this. I always type the trailing comma on multi-line arrays and hashes. To further support the VCS argument, allow me to mention
|
There are many other reasons for accidentally getting blamed for a line. The solution is not code formatting but better use of tools. Once you have blamed a line on someone, you should always look at the commit in question to see what changed. The result may be that you need to dig deeper. Vim's git plugin 'fugitive' can do this, for example. |
Ok agreed. I only said it was another reason for it the trailin commas in On Thursday, April 3, 2014, Matijs van Zuijlen notifications@github.com
Ernesto |
Semicolons are used all the time in various programming languages due to limitations in grammar parsers. And I'm sure there are other examples of "superfluous" characters being used in different contexts to work around tool limitations. Also, under that logic, we shouldn't bother on trimming trailing whitespace on our source code. They are invisible anyway, unless you configure your editor to show them, and the only real reason to avoid them is to have cleaner diffs. Oh, wait! But that reason is not good enough, because we also discourage using trailing commas in multi-line arrays in spite of having the advantage of giving cleaner diffs too. So recommending against trailing whitespace is inconsistent with discouraging trailing commas. Plus trailing commas have the added benefit mentioned by @edruder above, when refactoring/reordering a multi-line array literal. It's not just the cleaner diff argument. |
Notice how ruby doesn't have semicolon requirements. Trailing whitespace is superfluous just like trailing comas. I think the trend is that we should be avoiding superfluous characters. Your logic that |
I didn't say Ruby has semicolons, but thanks for pointing that out because I had not notice it until now ;) If the trend is avoiding superfluous characters, then we should be avoiding parenthesis around method arguments, both in method definitions and method calls. What makes a character superfluous exactly? Now seriously, I agree that parenthesis in the case mentioned above are not superfluous because they make things more readable. Well, I happen to think that the trailing comma in the case being discussed here makes things more readable too. So for me it's not superfluous. Superfluous is in many cases, a matter of opinion, not a fact written in stone. And again, trailing whitespace does not make anything more or less readable. The only real reason everyone agrees to discourage it, in nearly every known programming language, is because it gives cleaner diffs. But as in matters of religion, I think it'll be too hard to convince anyone here so vehemently opposing this change. We're not asking to encourage them, but merely to present it as an acceptable alternative, and perhaps mentioning the advantage of cleaner diffs and easier reordering of lines. It's not just a whim! |
Sorry to cause notifications on an old open issue. I've skimmed this and other discussions around trailing commas in multiline literals, and come up with some pros & cons: Pros:
Cons:
Note: I list aesthetics/readability in both because it's subjective and claimed by both people for & against. Please vote on this post. If you are IN FAVOR OF TRAILING COMMAS IN MULTILINE LITERALS, add the
|
@shull I'd like really like to vote "the guide should not make any recommendation except to be consistent within a project". |
Ever since I found prettier in the javascript world, I stopped worrying too much about these things. I realized that people probably argue for a code style because in order to adhere to it, they need to type the code in compliance with it. With something like So there's two key things to it:
These two features have made a difference for me, to the point that I no longer mind what things of how prettier rewrites my code are different than what I would've done. It rewrites it to a very readable format anyway, and that's all that matters. I wish there would be something like prettier for Ruby. It has changed my life regarding code styling issues. |
@mvz honestly I'm kinda with you on that 😏 @gnapse glad to hear you found a tool you like. The analog in ruby is of course rubocop but I bet you already knew that. The problem I have with auto-formatters is they are never smart enough. They rewrite your code for the purpose of "readability", but half the time they get it wrong. For example, there's a rule in rubocop that unwinds things like Anyway I'm glad you found one you like. Maybe one day rubocop will be good enough that I actually appreciate it. Just re-read this thread, noting how many people actually don't like trailing commas in multiline literals. As I suspected, it's not many (2 by my count). The problem is that one of them is @bbatsov, the owner of this project. So afaict he has simply steamrolled everyone (I mean freakin @steveklabnik weighed in for trailing commas ffs). This is a bummer because my colleague wants to treat this styleguide as a "community-consensus best practices" styleguide, but this issue in particular seems like evidence that it is more like "@bbatsov's personal preferences (maybe influenced somewhat by the community)" styleguide. Which is fine, but problematic when my colleague insists we follow it because it represents "best practices." Admittedly this is a single point, but I wonder how many others there are like it in here. Not sure how else to interpret what's going on here. |
@sdhull the problem with the examples you mention about the ruby code rewritter is precisely that it attempts to do too much, from what I see in those examples. prettier in js will never attempt to mess with the logic or change the AST of the code. It keeps the AST intact, just changes whitespace, indentation, etc. In my opinion, trying to enforce to that level, where a formatter thinks there's something wrong with Also, one of the problems with rubocop that you apparently did not consider in your response, is that it allows to be configured to the lowest level of detail. And one of the main advantages of prettier in js is that it allows for very little configuration, it is opinionated, and that's a good thing, because that's where it opens the door to reduce bike-shedding in code style matters, and focus on what really matters: the code itself, and its quality beyond code style. In my opinion, as long as an automated code formatter like prettier, spits out a reasonably organized and readable code, it does not matter what precise code style guide it adheres to. The fact that it removes the burden off my shoulders to manually format the code to comply is heaven for me. And finally, I totally agree with you that this Ruby style guide is in some degree very much about what @bbatsov prefers. It ultimately defines some of the issues. |
BTW @sdhull, you say the analogous to prettier in the ruby world is rubocop. Is rubocop able to rewrite the code according to a set of rules? Or is it just about detecting violations of those rules, for the developer to fix? If it can be used as a formatter, I wouldn't mind having a predefined set of rules applied (perhaps whatever rubocop has as defaults and using no config file whatsoever), and for me, that would be a good equivalent of prettier for the ruby world (provided the defaults match what's reasonably readable and organized code, which I'm sure they do). Bottomline is, the gist for what I consider the key features for prettier's success are: opinionated set of reasonable rules with very little room for customization + automatic rewrite of code according to those rules. Those two ingredients are a recipe for success IMO. |
Yes, it can do "some" auto-correction stuff. I couldn't find a good list of all the rules that are auto-correctable, but you may find it suites your needs (you could always leave the configuration vanilla, and/or disable enforcement of things you think are doing too much). As I mentioned, I find it obnoxious... but maybe I'll get used to it and learn to love it 🙄 |
I tried to have vim call |
@sdhull good to know rubocop has some capabilities to correct files. Next time I need to work on a more regular basis on a ruby project I'll definitely research how to achieve there what I do now with prettier. I cannot stress enough how it has changed my experience with code style issues. @lamont-granquist I'd never configure prettier to modify files on save. I have it configured to modify staged git changes on a pre-commit git hook, using lint-staged and husky. Again, these are all packages for a node ecosystem. It would be good to achieve a similar setup in a ruby/rails ecosystem using gems instead. |
I currently run rubocop on a precommit hook, but again due to occasional mangling I don't want to run |
@lamont-granquist You can perhaps limit the autocorrect to only correct the offenses in the |
@lamont-granquist Also, it turns out:
|
I see that there has been a lot of activity here since I last check that ticket. I'll close it as there was no recent activity, but I'll say I'm open to changing the wording of the rule to acknowledge the fact that many people seem to love those trailing commas. :-) |
I'm pretty disappointed in this outcome. Rejecting the suggestion is completely within your rights, but this style guide is marketed as being practical instead of idealistic, and a reflection of the general consensus of the Ruby community. This whole thread is evidence to the contrary, in my opinion.
|
The recent activity pinged me and I was just curious about what the main sticking points to the conversation were, and one recurring theme I saw (at least in the onset of the conversation), was that a comma with nothing after it seemed to make no sense. A comma represents a separation between two elements, so having nothing after the final comma was a separation between one element and nothing. This is not technically true. Although I'm not seeking to have this comment change everyone's mind, I would like to briefly explore computer science beyond strictly Ruby. When speaking about lists, you think in terms of array = [1, 2, 3, 4]
head == 1
tail == [2, 3, 4] So we can see that the head is an element within the list and the tail is the remainder of the list. But what does the tail look like when you loop through to the end of the list? array = [4]
head == 4
tail == [ ] So the tail of a list isn't nothing, it's something: an empty list. In this sense it shouldn't be odd to have a trailing comma. Those commas break up elements but, more importantly, they break the head of a list from its tail. Let's expand that first example array to see this concept better. [1, [2, [3, [4, []]]]] I'll admit that such a representation looks ridiculous. But ultimately that's an accurate abstract representation of the list But why should anyone think in these terms? I mean, the above expanded array is confusing and an eyesore. Well, one big benefit is with algorithms, especially those that use recursion to define them. Let's say you were writing an algorithm to reverse a list. This algorithm might need to be implemented in multiple programming languages, so constructing pseudo-code to represent the abstract algorithm will be more helpful than just writing some language-specific code. If your pseudo-code is having to worry about list lengths, null terminators, etc, then it's not clean and it likely won't be implemented well. Instead, you would use the abstract concept of a list I've described above:
The def reverse([], acc), do: acc
def reverse([head | tail], acc), do: reverse(tail, [head | acc]) That's valid Elixir code and it will return a reversed list. What's nice here is that we can check a single terminal condition (an empty list) and then return our accumulator, So in my mind, the trailing comma makes perfect sense. The thing after the final element in a list is the empty list because there's always that implicit tail. |
I'm also in favor of trailing commas and at least relaxing the wording in the style guide. For my own part, so many decisions about coding style come down to paper cuts. Things like the diff being cleaner and not having to worry about shuffling commas when I change code are not a big deal, but they make the developer experience incrementally better. I'm a pretty distractible person, so these relatively minor "ticks" of distraction really add up over time. I find the aesthetics arguments a little frustrating. It basically boils down to "I'm not used to it". Whenever there has been a style choice I've disliked on aesthetic grounds, I completely forget my distaste after about two weeks of using it. I had the same reaction to trailing commas when I first saw them. It seemed just "wrong". Ultimately, the concrete benefits outweighed my aesthetic complaints and now I couldn't care less. The argument that an editor integration makes it a trivial issue is a little dismissive. While it is nice to rely on automated tools as much as possible, that can come with its own caveats that need to be handled. In this case, even though it makes me aware that I have a syntax error, I still have to go back and fix it, which is another distraction. And expecting everybody to have an integration in the first place is a little unreasonable. If I spent all the time it would take to implement all of the integrations everybody thinks I should have, I'd never get anything done. Let alone getting them all to play nice together. |
Given the controversy regarding this style, perhaps the warning should be removed? |
Warnings come from RuboCop, not from the style guide. 😉 I'll add here two things - the survey we did last year clearly indicated that the majority of the people (at least those who participated) prefer the currently suggested style (no trailing commas). To me this completely kills the case that some people have been trying to build - namely that the guideline doesn't reflect practices in the wild. I've also said a couple of times that trailing comma guidelines can be revised (softened a bit), although this is not going to change the RuboCop defaults, which I assume is the main problem of the people unhappy with the current situation. 😄 PRs to update the wording are welcome; eventually I might get to doing this myself. P.S. I'm 100% sure that if we flip this guideline, then I'll be on a similar thread with all the people who hate trailing commas. It's really happy to appease everyone. :D |
@scott-lydon RuboCop could consider introducing the second style for this cop if there is evidence that certain projects are using that other style consistently. For proof harvesting, I'd suggest analyzing https://github.com/eliotsykes/real-world-rails and https://github.com/jeromedalbert/real-world-ruby-apps. Surely such evidence could also affect the style guide given that the number of projects is high enough. Those are some (personal) general rules I apply when working on adjusting cop styles (I contribute to Trailing commas are no exception to those rules. If you're willing to conduct such research - you are welcome! |
This style has been supported by RuboCop for ages, it's simply not the default. There are actually several different cops for the various cases in which you can use a trailing comma (e.g. https://docs.rubocop.org/rubocop/1.11/cops_style.html#styletrailingcommainarrayliteral). |
I'm surprised things like cognitive overhead, and uniformity or consistency don't come up here? To me at least, "it looks weird" doesn't seem like a very solid argument against this? Why would you assume the hash is missing an entry or is incomplete? There are no bugs, the test suite passes, and nobody is complaining about something missing or being broke? A PR Review would probably involve someone noticing entries randomly being deleted or a test suite failing? But your first thought is "code is missing" ? However... Not having this one single comma now adds unnecessary overhead every time you touch this code. While these may be minor, and take all of 5 seconds to fix; It is still overhead. Something a style guide should be trying to reduce?
Why is the last entry of an array/hash this special? Shouldn't we be trying to reduce exceptions in our coding styles?
Anyways. I know my two cents mean zero in this beating of a dead horse. Just wanted to air out my disagreement on this. |
Just expressing my ongoing frustration after months of using Standard Ruby. |
Title says it all. Please discuss.
The text was updated successfully, but these errors were encountered: