Rubocop: TrailingCommaInArguments requires comma after single argument

[CyborgMaster]

I would love to turn this on, because I like the trailing comma, the problem
is that it _always_ wants a comma, including if it is just a long single
argument.

---

Expected behavior

No offense

Actual behavior

test/test_helper.rb:355:8: C: Style/TrailingCommaInArguments: Put a comma after the last parameter of a multiline method call.  (https://github.com/bbatsov/ruby-style-guide#no-trailing-params-comma)
       editor.insertContent('#{with}')"
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Steps to reproduce the problem

    page.execute_script(
      "editor = tinymce.editors.find(function(editor) {
         return $(editor.getElement()).hasClass('#{id}')
       });
       editor.setContent('');
       editor.insertContent('#{with}')"
    )

This is extremely important! Providing us with a reliable way to reproduce
a problem will expedite its solution.

RuboCop version

Include the output of rubocop -V:

➜  allynova git:(rubocop) ✗ be rubocop -V
0.42.0 (using Parser 2.3.1.2, running on ruby 2.3.1 x86_64-darwin15)

[Drenmi]

I guess there is some ambiguity here, because the configuration option is named EnforcedStyleForMultiline. I prefer the behaviour you suggested, but a bit weary about making the change because of that.

[CyborgMaster]

Makes sense. I assume that option name is referring to something like this:

method(
  arg1,
  arg2,
  arg3,
)

So maybe it should only apply if there is more than one argument?

[Drenmi]

Yeah. My fear is that the name will imply more than it's giving away, but also EnforcedStyleForMultilineMultiargument sounds kind of long. 😅

[CyborgMaster]

As long as we properly comment it in default.yml I don't think it would be too bad. If people are curious, they can look up the details. I don't think the benefits outweigh the slight confusion it might cause. I also think this is the behaivor most people would want anyways when turning on this cop, so this better follows the Principle of Least Surprise.

[jonas054]

The idea that prompted us to make a trailing comma cop was that if you add a parameter or element at the end of a list you get an unwanted diff on the former last line because you had to add a comma there. Single argument or single element are not special cases in that line of reasoning. Do you have a different idea about what the trailing comma is for?

[CyborgMaster]

It lets you reorder them without worrying about the trailing comma (which makes sense with named parameters).

I understand what you are saying about single argument not being a special case, but I would still argue that it is. I still think that this example looks really weird with a comma:

page.execute_script(
      "editor = tinymce.editors.find(function(editor) {
         return $(editor.getElement()).hasClass('#{id}')
       });
       editor.setContent('');
       editor.insertContent('#{with}')",
    )

while if it is a list it looks good:

method(
  arg1,
  arg2,
  arg3,
)

There are a lot of functions that only take one argument by definition and sometimes that argument can be long. Because there are no plans to ever add more arguments, the trailing comma is unneeded.

[jonas054]

this example looks really weird with a comma

Isn't that because of the contents of those lines? That it's JavaScript code. In the general case I see no evidence that a trailing comma looks weird.

Because there are no plans to ever add more arguments, the trailing comma is unneeded.

RuboCop can't guess what the plans are. :smile:

[CyborgMaster]

That is javascipt code, I can find a better example:

      east_hour = current_time.in_time_zone(
        ActiveSupport::TimeZone['Eastern Time (US & Canada)'] <--- want's a comma here
      ).hour

RuboCop can't guess what the plans are. 😄

exactly, that's why I'm suggesting that it's more lenient, leaving it up to me.

[jonas054]

Here's what we could do. We could add a new style, comma_for_multiple_args, to the existing list.

Style/TrailingCommaInArguments:
  # If `comma`, the cop requires a comma after the last argument, but only for
  # parenthesized method calls where each argument is on its own line.
  # If `comma_for_multiple_args`, the cop behaves like `comma` except when
  # there's only one argument, in which case it requires that there's no
  # trailing comma.
  # If `consistent_comma`, the cop requires a comma after the last argument,
  # for all parenthesized method calls with arguments.
  EnforcedStyleForMultiline: no_comma
  SupportedStyles:
    - comma
    - comma_for_multiple_args
    - consistent_comma
    - no_comma

Adding a new parameter, EnforcedStyleForMultilineMultiargument, as mentioned above, would increase the complexity of the cop much more.

[CyborgMaster]

I love it! If you guys would be willing to do that, I would be grateful.

[wjbuys]

Does it make sense to apply this to the TrailingCommaInLiteral cop too?

[CyborgMaster]

To me it does, I would be all for it.

[timurb]

Do you think you could add a parameter for minimum hash size in elements/lines after which the cop will be applied? Or that's going to add much complexity too?

[onebree]

Hi everyone. I know this is labeled hacktoberfest, but would I be able to tackle this one? This is the exact configuration I'm looking for wrt trailing commas.

[jonas054]

Yes of course! October is over.

[bbatsov]

There hasn't been much activity on this ticket and our Core Team is spread too thin on so many tasks, so I'll just close it for the sake of having a cleaner lists of tasks to focus on. We'd gladly review a PR, but it's unlikely that'd we're going to tackle this ourselves in the foreseeable future.