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

Allow trailing comma in tuples #789

Closed
timotheeguerin opened this issue Jul 28, 2022 · 5 comments
Closed

Allow trailing comma in tuples #789

timotheeguerin opened this issue Jul 28, 2022 · 5 comments
Assignees
Labels
design:needed A design request has been raised that needs a proposal

Comments

@timotheeguerin
Copy link
Member

timotheeguerin commented Jul 28, 2022

We had a bug in the formatter that was adding a trailing comma to multi line tuple but this isn't something we actually support.

From bug #787, bug fix the formatter to remove the trailing comma but question is should we allow the other way around and have the formatter add it automatically.

TS/JS support that and prettier (at least our config) will automatically add one. This make it nicer to work with multi line arrays, reducing the number of line changed in diff when adding a new entry at the end and make it easier to move items around.

@timotheeguerin timotheeguerin added the design:needed A design request has been raised that needs a proposal label Jul 28, 2022
@ghost ghost added the Needs Triage label Jul 28, 2022
@timotheeguerin timotheeguerin changed the title Aallow trailing comma in tuples Allow trailing comma in tuples Jul 28, 2022
@nguerrera
Copy link
Contributor

nguerrera commented Jul 28, 2022

+1 we should allow it.

I am less opinionated on what the formatter should do, but I think following the prettier defaults makes sense.

@nguerrera
Copy link
Contributor

nguerrera commented Jul 28, 2022

I'm thinking we should maybe consider allowing trailing in all lists and leave it to the formatter if we have opinions about when not to use it (like in a single line).

When I refactored everything to go through parseList helper, I preserved the cases where trailing delimiter is not allowed, but there's an explicit trailingDelimeterIsValid: false for that. We could consider removing that and saying it's always valid. We should still add formatting tests for all the lists this impacts and update the grammar if we go down this route.

@nguerrera nguerrera self-assigned this Aug 3, 2022
@nguerrera
Copy link
Contributor

nguerrera commented Aug 3, 2022

I made examples of all the places:

Playground Link

Model properties and interface members are also OK today, but not shown.

@nguerrera
Copy link
Contributor

Proposal

  • Allow trailing comma in all lists. (Make everything NOT OK here OK.)
  • Allow formatter to do whatever comes easiest and is most consistent with JS/TS prettier defaults
  • Don't implement any formatter knobs yet. If that is desired, consider it as a separate, lower priority issue.

Implementation issue notes:

  • Delete trailingDelimiterIsValid, and behave as though it was always true.
  • Add parser tests for all cases.
  • Add formatter tests for all cases.
  • Update language grammar

@nguerrera
Copy link
Contributor

Approved on 8/3 design meeting. #809 tracks implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design:needed A design request has been raised that needs a proposal
Projects
None yet
Development

No branches or pull requests

3 participants