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

Don't add trailing comma in object rest spread #3313

Merged
merged 1 commit into from Nov 23, 2017

Conversation

Projects
None yet
4 participants
@duailibe
Member

duailibe commented Nov 23, 2017

For some reason the rest property in objects in typescript is a ExperimentalRestProperty node in the AST and we were not checking for it when printing trailing commas in object expressions.

Fixes #3311

@ikatyang

This comment has been minimized.

Member

ikatyang commented Nov 23, 2017

I'm still not sure what we should do here since the --trailing-comma all docs said:

"all" - Trailing commas wherever possible (including function arguments). This requires node 8 or a transform.

and the trailing comma is valid in TypeScript.


And if the new TS/JS spec allowed more trailing comma cases, how should we deal with it?

EDIT: maybe more es6, es7, es8, ts2.5, ts.2.6, ts2.7 options? 😂

@duailibe

This comment has been minimized.

Member

duailibe commented Nov 23, 2017

I'm not opposed to leaving it as is, in that case we should add to arrays and functions.. it's inconsistent right now.

I'm not sure the added complexity is worth though (checking the options + checking the parser) because they're not useful at all in those cases IMO.

EDIT: I understand your point now.. I guess the es5 option should only apply when using babylon? And then we should treat trailingComma as a boolean for TS/flow.

EDIT: maybe more es6, es7, es8, ts2.5, ts.2.6, ts2.7 options? 😂

Don't forget about flow!

@duailibe

This comment has been minimized.

Member

duailibe commented Nov 23, 2017

Although leaving this comma would be strictly correct considering "apply wherever possible", I think it wouldn't add much value and we can reason about new cases separately if/when they appear.

@ikatyang

This comment has been minimized.

Member

ikatyang commented Nov 23, 2017

Since the three trailing comma cases below are all valid TypeScript and we didn't print trailing commas for two of them, removing the trailing comma in the last case should be fine and more consistent.

Prettier 1.8.2
Playground link

--parser typescript
--trailing-comma all

Input:

const [
  longKeySoThisWillGoOnMultipleLines,
  longKeySoThisWillGoOnMultipleLines2,
  longKeySoThisWillGoOnMultipleLines3,
  ...rest,
] = something;

const {
  longKeySoThisWillGoOnMultipleLines,
  longKeySoThisWillGoOnMultipleLines2,
  longKeySoThisWillGoOnMultipleLines3,
  ...rest,
} = something;

function func(
  longKeySoThisWillGoOnMultipleLines,
  longKeySoThisWillGoOnMultipleLines2,
  longKeySoThisWillGoOnMultipleLines3,
  ...rest,
) {
  // something
}

Output:

const [
  longKeySoThisWillGoOnMultipleLines,
  longKeySoThisWillGoOnMultipleLines2,
  longKeySoThisWillGoOnMultipleLines3,
  ...rest
] = something;

const {
  longKeySoThisWillGoOnMultipleLines,
  longKeySoThisWillGoOnMultipleLines2,
  longKeySoThisWillGoOnMultipleLines3,
  ...rest,
} = something;

function func(
  longKeySoThisWillGoOnMultipleLines,
  longKeySoThisWillGoOnMultipleLines2,
  longKeySoThisWillGoOnMultipleLines3,
  ...rest
) {
  // something
}

@duailibe duailibe merged commit 742a5c3 into prettier:master Nov 23, 2017

3 checks passed

codecov/patch Coverage not affected when comparing a1d878a...f8a0ad9
Details
codecov/project 97.06% remains the same compared to a1d878a
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@duailibe duailibe deleted the duailibe:ts-trailing-comma-obj-rest branch Nov 23, 2017

lipis added a commit to lipis/prettier that referenced this pull request Nov 24, 2017

Merge branch 'master' into analytics
* master:
  Don't add trailing comma after object rest spread in TypeScript  (prettier#3313)
  fix(cli): respect `--ignore-path` when using `--stdin-filepath` (prettier#3309)
  Fix infinite recursion in playground (prettier#3305)
  chore: setup markdown formatting (prettier#3224)

@suchipi suchipi added this to the 1.9 milestone Nov 28, 2017

macklinu added a commit to macklinu/react-fns that referenced this pull request Jan 24, 2018

Update to Prettier v1.10.x
Running `yarn format` after updating results in changes to Mailto and
Sms components as a result of prettier/prettier#3313 being shipped in
Prettier v1.9.

@macklinu macklinu referenced this pull request Jan 24, 2018

Merged

Update to Prettier v1.10.x #69

macklinu added a commit to macklinu/react-fns that referenced this pull request Feb 5, 2018

Update to Prettier v1.10.x
Running `yarn format` after updating results in changes to Mailto and
Sms components as a result of prettier/prettier#3313 being shipped in
Prettier v1.9.

jaredpalmer added a commit to jaredpalmer/react-fns that referenced this pull request Apr 13, 2018

Update to Prettier v1.10.x (#69)
Running `yarn format` after updating results in changes to Mailto and
Sms components as a result of prettier/prettier#3313 being shipped in
Prettier v1.9.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment