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

Thought: Enable trailing commas in Hash Literals (per Ruby 3.1 addition of implicit hash values) #611

Open
thewatts opened this issue Mar 5, 2024 · 3 comments
Labels
rule change 👩‍⚖️ Suggest a style guide rule change

Comments

@thewatts
Copy link
Contributor

thewatts commented Mar 5, 2024

First off, I love this project, and it's brought significant joy to my life and the lives of my team.

Truly - thank you for the significant amount of hard work, and hard decisions, you all have had to make.

I'm aware that this issue (for other reasons) has been talked about before.

My goal with this issue isn't to bike-shed, or cause stress. I just wanted to make a suggestion here, given some (somewhat) recent changes with what's possible in Ruby, and how I feel the current trailing commas rule causes some visual friction when writing/reviewing code.

This is just my opinion here, and I'm in no way saying everyone should have it - but I feel like others may find it valid.

My premise: enforcing "avoiding comma after last item of a hash" (as opposed to the opposite), hurts code readability with implicit hash values.

Here's a example below:

Note: I understand that the below code could be written a different way - I'm just trying to demonstrate a simple example of how this popped up for us

Say we have some class that builds up a hash value, and it just so happens that:

  1. The file is large enough to where the entire file can't fit on a single page
  2. The values of the hash are calling methods (not required - but adds to the readability difficulty I'm speaking of)
  3. Except for the last value of the hash, which pulls its value implicitly from a method on the class
class PersonSerializer
  def initialize(person)
    @person = person
  end

  ### some other methods

  def to_h
    {
      name: @person.name,
      age: @person.age,
      date_of_birth: @person.date_of_birth,
      favorite_color:
    }
  end

  ### some other methods

  ### and some more methods

  private

  def favorite_color
    @person.favorite_color || "Favorite color not known"
  end
end

To my eyes, when scanning this code, it looks like favorite_color is missing its value (particularly if I don't know - due to the size of the file, etc, that the favorite_color method exists). I can't help but think that there's a problem with the file/code when reading it.

Whereas the opposite:

class PersonSerializer
  def initialize(person)
    @person = person
  end

  ### some other methods

  def to_h
    {
      name: @person.name,
      age: @person.age,
      date_of_birth: @person.date_of_birth,
      favorite_color:,
    }
  end

  ### some other methods

  ### and some more methods

  private

  def favorite_color
    @person.favorite_color || "Favorite color not known"
  end
end

My eyes see the , and my brain automatically assumes that we're implicitly referencing a method/variable somewhere - and can continue reviewing the file without my brain hitting the breaks.


Additionally, here's an example diff from a PR in a project:

CleanShot 2024-03-05 at 09 32 13@2x


I'm unsure if this resonates with everyone, and if you all disagree — I completely understand. I just thought I'd mention it, as it has come up for me multiple times as a code writer and reviewer.

Note: Edited to remove screenshots, as it looks like the code samples have embedded as scrollable, and helps demonstrate what I'm speaking of

@searls
Copy link
Contributor

searls commented Mar 5, 2024

Hey Nathaniel, thanks for the kind note. I appreciate your perspective and see where you're coming from.

Personally, seeing foo:, doesn't look any more or less jarring than a naked foo: (in truth, they're both unsettling to my eyes). I've been using this shortcut in earnest for a few months, though, and I generally prefer anything that lets me be terser.

Ultimately, I'm still 👎 on this, for these reasons:

  1. Generally Standard strives not to make people type more than necessary, as it will lead to people relying on the auto-formatter to do work when the goal of standard is to pretty much "look how you'd type it" once you're used to the style even if the auto-fixer weren't available to you (and I resent having to type more to make the computer happy; it's part of why I switched from Java to Ruby)
  2. Because we don't enforce trailing commas for other literals, method signatures, and calls, I believe it would be inconsistent to introduce it here and only here
  3. This feature is new enough that I've had the policy of wait-and-see before reacting to it. People opened issues that we should ban numbered parameters right away, for instance, but until a bunch of people are using something and seeing a problem, I'm inclined to wait to see how folks feel about the feature after we've all lived with it for a while

Thanks again for submitting this. Will leave it open for others who want to comment

@searls searls added the rule change 👩‍⚖️ Suggest a style guide rule change label Mar 5, 2024
@krystof-k
Copy link

krystof-k commented Apr 8, 2024

Yes. And everywhere else. Please!

Quoting @zacheryph from rubocop/ruby-style-guide#273:

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?

  • every time you look at a diff you might be looking at noise
  • every time you append an entry you need to alter a line unrelated to your change
  • every time you remove an entry you might need to alter a line unrelated to your change
  • every time you shuffle/reorder that array/hash you might need to shuffle a comma around

Why is the last entry of an array/hash this special? Shouldn't we be trying to reduce exceptions in our coding styles?

  • all entries get a comma... EXCEPT the last one, you are special.
  • Add code. EXCEPT to the end of an array/hash. You might need to touch lines you aren't concerned about

I'm really frustrated about this after months of adopting Standard Ruby.

@bbatsov
Copy link

bbatsov commented Apr 8, 2024

Yes. And everywhere else. Please!

Quoting @zacheryph from rubocop/ruby-style-guide#273:

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?

  • every time you look at a diff you might be looking at noise
  • every time you append an entry you need to alter a line unrelated to your change
  • every time you remove an entry you might need to alter a line unrelated to your change
  • every time you shuffle/reorder that array/hash you might need to shuffle a comma around

Why is the last entry of an array/hash this special? Shouldn't we be trying to reduce exceptions in our coding styles?

  • all entries get a comma... EXCEPT the last one, you are special.
  • Add code. EXCEPT to the end of an array/hash. You might need to touch lines you aren't concerned about

I'm really frustrated about this after months of adopting Standard Ruby.

Someone will always be frustrated by some stylistic decision. That's pretty much unavoidable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule change 👩‍⚖️ Suggest a style guide rule change
Projects
None yet
Development

No branches or pull requests

4 participants