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

FR: --fix-trailing-commas to always add trailing commas #753

Open
srawlins opened this Issue Nov 16, 2018 · 10 comments

Comments

Projects
None yet
4 participants
@srawlins
Copy link
Member

commented Nov 16, 2018

Motivation

Large function calls (e.g. constructors), such as the large annotation calls in Angular, can be formatted 3 (three) (un, deux, trois) ways, based on the commas:

@Component(selector: 'button', templateUrl: 'button.html', directives: [
  ItemOne,
  NoTrailingComma
], providers: [
  providerOne,
  Provider(FooBuilder, useFactory: provideFooBuilder)
], exports: [
  ExportOne,
  ExportTwo
])
int a;

@Component(
  selector: 'button',
  directives: [
    ItemOne,
    TrailingComma,
  ],
  providers: [
    providerOne,
    Provider(FooBuilder, useFactory: provideFooBuilder),
  ],
  exports: [
    ExportOne,
    ExportTwo,
  ],
  templateUrl: 'button.html',
)
int b;

@Component(
    selector: 'button',
    directives: [
      ItemOne,
      TrailingComma,
    ],
    providers: [
      providerOne,
      Provider(FooBuilder, useFactory: provideFooBuilder),
    ],
    exports: [
      ExportOne,
      ExportTwo,
    ],
    templateUrl: 'button.html')
int c;

The Component constructor calls are semantically exactly the same. The CST differences:

  1. In the first case, none of the inner list literals have trailing commas. The result is ] positioned at column 0.
  2. In the second case, the inner lists have trailing commas, and the call to Component has a trailing comma. Also templateUrl, which is not given a List literal, has been listed out-of-alphabetical-order, as the second argument. The result is ] positioned at column 2, and each argument starting on its own line.
  3. In the third case, the inner lists have trailing commas, but the call to Component has not. The result is ] positioned at column 4, with each argument starting on its own line.

OK, I get it, the trailing comma situation results in different code. I don't want to address that here.

The ask

What I want is a --fix feature, like --fix-trailing-commas, which adds trailing commas to list literals and function calls which are already being split into multiple lines. For example I would not want a trailing comma added to print('hi'), or to the single-lined Provider() above.

The goal is to unify all long method calls and list literals to the second case above, which I think today is most common anyway, and is the most visually appealing.

And actually, in my example I've severely trimmed the list of directives and such. The code I took this from had a dozen or so directives, so it was always going to be multiple lines.

(Also Map literals and Set literals I imagine...)

@natebosch

This comment has been minimized.

Copy link
Member

commented Nov 18, 2018

What about method signatures at the definition point? They also allow trailing commas that impact formatting? Further complicated by optional positional or named arguments...

I agree that making the edits manually and keeping things consistent within a project is tough now and could be automated. What I worry about is whether this is opening dartfmt up for too much configurability and style arguments - there are at least 3 situations where this could be applied - signatures, collection literals, function calls - who decides which of the 3 get it?

@srawlins

This comment has been minimized.

Copy link
Member Author

commented Nov 19, 2018

Rather than decide on which ones, I'd be happy if --fix-trailing-commas added trailing commas everywhere they are allowed (in line-wrapped contexts).

@Empty2k12

This comment has been minimized.

Copy link

commented Mar 26, 2019

Rather than decide on which ones, I'd be happy if --fix-trailing-commas added trailing commas everywhere they are allowed (in line-wrapped contexts).

That would be the most sensible option. The (in line-wrapped contexts) is very important.

What's the status on this issue?

@srawlins

This comment has been minimized.

Copy link
Member Author

commented Mar 26, 2019

I think the next step is for @munificent needs to triage. WDYT, @munificent?

@munificent

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

Anything related to trailing commas and the resultant formatting differences is a huge rathole that I don't have time to crawl into right now. Sorry. :(

@srawlins

This comment has been minimized.

Copy link
Member Author

commented Mar 27, 2019

Hmm, I don't think the resultant formatting differences is related to this issue. I just thought you applied labels to issues as they came in, in a short triage process. Not a P1 from my perspective though.

@munificent

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

OK, labeled. :)

@srawlins

This comment has been minimized.

Copy link
Member Author

commented Mar 28, 2019

😛 I'll take that as a "This seems like an acceptable idea. I'm open to pull requests 😁 "

@munificent

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

I'll take that as "I won't be offended if the PR languishes indefinitely." :)

@srawlins

This comment has been minimized.

Copy link
Member Author

commented Mar 28, 2019

Well played, sir!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.