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

Trailing Commas in Function Param Lists #7279

Closed
ewinslow opened this issue Feb 28, 2016 · 10 comments · Fixed by #8942
Closed

Trailing Commas in Function Param Lists #7279

ewinslow opened this issue Feb 28, 2016 · 10 comments · Fixed by #8942
Labels
Committed The team has roadmapped this issue ES Next New featurers for ECMAScript (a.k.a. ESNext) Fixed A PR has been merged for this issue Help Wanted You can do this Suggestion An idea for TypeScript

Comments

@ewinslow
Copy link

https://jeffmo.github.io/es-trailing-function-commas/

This proposal is Stage 3 according to https://github.com/tc39/ecma262, so seems like its at an appropriate stability for TypeScript team to implement it.

This would allow function definitions and calls in the following style:

function foo(
  bar: Bar, // Comment about bar
  baz: Baz, // Comment about baz
) {
  // Implementation...
}

foo(
  bar,
  baz,
);

Benefits: https://github.com/jeffmo/es-trailing-function-commas/blob/master/proposal_presentation_slides.pdf

  • Helps with VCS authorship attribution in case of added/removed parameters
  • Consistency with object and array literals
  • Lintable (if you don't like the style, can ban it in your codebase with a linter)
  • Supported by other languages

In addition to all of the above, I suspect the extra "verbosity" of TypeScript's inline types makes this feature even more attractive in TypeScript than in vanilla JS. I feel this particularly in class constructors, which can also have private annotations, and possibly a readonly annotation coming down the pipe.

class Foo {
  constructor(
    private bar: Bar = new BasicBar(),
    private baz: Baz = new BasicBaz(),
  ) {}
}
@RyanCavanaugh RyanCavanaugh added Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels Feb 28, 2016
@mhegazy mhegazy added Committed The team has roadmapped this issue and removed In Discussion Not yet reached consensus labels Mar 8, 2016
@RyanCavanaugh
Copy link
Member

Approved for whatever the Stage 3 proposal says

@mhegazy
Copy link
Contributor

mhegazy commented Mar 8, 2016

PRs are welcomed of course. the change should be limited to the parser, so would be a rather easy change to make.

@RyanCavanaugh RyanCavanaugh added the Help Wanted You can do this label Mar 8, 2016
@DanielRosenwasser
Copy link
Member

Actually, signature help and overload resolution could potentially be affected by this.

@nojvek
Copy link
Contributor

nojvek commented May 22, 2016

Is anyone taking on this? If not I can. I spent almost my entire weekend playing with the parser and scanner.

@DanielRosenwasser, what do you mean by signature help and overload resolution?

@mhegazy
Copy link
Contributor

mhegazy commented May 23, 2016

@nojvek there is a PR for this already in #7747. if you want to take that over and respond to code review comments we can get it in.

@DanielRosenwasser
Copy link
Member

@nojvek signature help is the little tooltip that shows up to show the parameters & their types when you call a function.

image

I mean that we specifically check if you have a trailing comma to see if there is a "better" overload in some cases.

For instance, here's a call to localeCompare with one argument:

image

Here's a call with one argument and a trailing comma:

image

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented May 23, 2016

So in general, my warning is that we can't do that anymore if we support trailing commas in argument lists. If we do that, we'd impair the experience of anyone who wants to use trailing commas in calls.

@mhegazy
Copy link
Contributor

mhegazy commented May 23, 2016

do not think this is an issue we should change the design of signature help for. you will get the pop up, you could decide to dismiss it an keep the comma.

@mhegazy mhegazy added the ES Next New featurers for ECMAScript (a.k.a. ESNext) label Jun 6, 2016
@ghost ghost closed this as completed in #8942 Jun 7, 2016
@mhegazy mhegazy added the Fixed A PR has been merged for this issue label Jun 7, 2016
@ewinslow
Copy link
Author

ewinslow commented Jun 8, 2016

Woohoo!

@basarat
Copy link
Contributor

basarat commented Jul 6, 2016

Personal opinion to any users : If you are thinking about adding a trailing comma to a function you might want to reconsider and change the function parameters to a single object https://basarat.gitbooks.io/typescript/content/docs/tips/functionParameters.html 🌹

davidyaha added a commit to davidyaha/graphql-subscriptions that referenced this issue Sep 2, 2016
* Added pubsub argument to SubscriptionManager to allow for different kinds of PubSub Engines
* Updated typescript dependencies to 2.0.0 to allow for the current graphql typings to work. see microsoft/TypeScript#7279
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Committed The team has roadmapped this issue ES Next New featurers for ECMAScript (a.k.a. ESNext) Fixed A PR has been merged for this issue Help Wanted You can do this Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants