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

Template literals: Don't break on identifiers but break if comments #3299

Merged
merged 2 commits into from Nov 21, 2017

Conversation

Projects
None yet
3 participants
@duailibe
Member

duailibe commented Nov 21, 2017

Context: #3280 (comment)

@suchipi

Looks good to me. I wonder if we should throw "short" member chains in, too (for some definition of short...). But that could be a different PR.

@SimenB

This comment has been minimized.

Contributor

SimenB commented Nov 21, 2017

I love your responsiveness to issues. Thanks you! ❤️

What about throwing in some more examples of formatting? More as something to compare against if it changes in the future than anything.

Here's a couple of examples from Jest's migration to 1.8 (facebook/jest#4852):

descirbe('something', () => {
  test(`{pass: false} expect(${small}).toBeGreaterThanOrEqual(${big})`, () => {});  
})

throw new Error(`pretty-format: Option "theme" has a key "${key}" whose value "${value}" is undefined in ansi-styles.`,)

That last one is really long, but breaking it on only one identifier and not the other makes is difficult to read, as it's not natural (I've never seen a human write code that way)

Anecdotally, one thing I came over in a personal project where I would like a break, but the break is wrong

class Class {
  something() {
    // This would be WAY better if it took the whole `time.unit.charAt(0)` on a new line instead of just the `0`
    // Not breaking at all is better than that
    return () => {
      const timeQuery = time
      ? `time:(from:now-${time.value}${time.unit.charAt(0)},mode:quick,to:now)`
      : "";
    }
  }
}
@duailibe

This comment has been minimized.

Member

duailibe commented Nov 21, 2017

Thanks for the kind words and the examples! ❤️

The last one is a really trick case because we have two separate places to break:

`time:(from:now-${time.value}${
  time.unit.charAt(0)
},mode:quick,to:now)`

`time:(from:now-${time.value}${time.unit.charAt(
  0
)},mode:quick,to:now)`

And it currently tries to fit everything in the line and break in "last" possible place. So we have no way of telling "breaking here is preferable than breaking over there". This is another case that would benefit from #3014.

@SimenB

This comment has been minimized.

Contributor

SimenB commented on tests/template_literals/expressions.js in 2fdb17f Nov 21, 2017

descirbe - my bad, haha

@duailibe

This comment has been minimized.

Member

duailibe commented Nov 21, 2017

Actually, my bad.. this one is a different case. Including a softline for CallExpressions in the template literals expressions works for your example, but breaks in another one:

const foo = `something:${JSON.stringify({
  a,
  b,
  c
})}`

// turns into:

const foo = `something:${
  JSON.stringify({
    a,
    b,
    c
  })
}`

@duailibe duailibe merged commit 759953e into prettier:master Nov 21, 2017

3 checks passed

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

@duailibe duailibe deleted the duailibe:identifiers-in-template-literals branch Nov 21, 2017

@suchipi suchipi added the release:1.9 label Nov 28, 2017

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

IvanGoncharov added a commit to APIs-guru/graphql-js that referenced this pull request Jan 3, 2018

Update prettier
Code changes are due to: prettier/prettier#3299

@IvanGoncharov IvanGoncharov referenced this pull request Jan 3, 2018

Merged

Update prettier #1191

leebyron added a commit to graphql/graphql-js that referenced this pull request Jan 8, 2018

Update prettier (#1191)
Code changes are due to: prettier/prettier#3299
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment