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

Add support for flow typecast comments #5280

Merged
merged 7 commits into from Oct 23, 2018

Conversation

Projects
None yet
4 participants
@swac
Contributor

swac commented Oct 19, 2018

This attempts to fix the bug described in #719. Previously, Prettier was removing parentheses in expressions like:

new (window.Request /*: Class<Request> */)("")

turning them into:

new window.Request /*: Class<Request> */("")

which no longer satisfies Flow's syntax. With this change, we detect these typecast expressions and attempt to keep the parentheses intact in such cases.

This is my first time contributing to Prettier, so I'm probably making some egregious mistakes!

  • I’ve added tests to confirm my change works.
  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory)
  • I’ve read the contributing guidelines.

@swac swac referenced this pull request Oct 19, 2018

Closed

Flow Typecast Comments #719

@j-f1

Thank you for contributing! I’ve got a couple questions/suggestions:

Show resolved Hide resolved src/language-js/flow-comments.js Outdated
Show resolved Hide resolved src/language-js/flow-comments.js Outdated
Show resolved Hide resolved src/language-js/flow-comments.js Outdated
Show resolved Hide resolved src/language-js/printer-estree.js Outdated
Allow whitespace between comment start and colon
Also rename flow-comments.js to utils.js
@lydell

This comment has been minimized.

Collaborator

lydell commented Oct 19, 2018

We need to be careful with that whitespace trimming though, because newlines do not seem to be supported. A goal of Prettier is to never break your code. Here's a case where it could break: https://flow.org/try/#0PQKgBAAgZgNg9gdzCYAoVxhgIIGMAuArgIYxj4CeADgKZjEB2Dc+x+AlnAwFyoBuxAE70woMGG5gGhALYAjGsJRgAvGACs6TGAByLEZVr0mLNpx78hYOaJCpJAfmnzFyLGs3aAKoIphBNDJwfOwMAObkABZ0DDQIMKF0ClBwAVF0uHDwDGCAyARgFHCEAOQwZGE0+AbUdIqCqQCE6ALCuKoiADzWQA

/* @flow */

// Actual type annotation:
var a /*  : number */ = 5

// Not a type annotation:
var b /*
: ?number */ = 5
// Try removing the newline before the colon – you'll get a type error!

var c = a < b
@swac

This comment has been minimized.

Contributor

swac commented Oct 19, 2018

@lydell, nice catch. I've updated this to fix that whitespace issue, and added some tests to demonstrate the behavior.

Show resolved Hide resolved src/language-js/utils.js Outdated
Show resolved Hide resolved src/language-js/utils.js Outdated
Show resolved Hide resolved src/language-js/printer-estree.js Outdated
Show resolved Hide resolved src/language-js/printer-estree.js Outdated
Show resolved Hide resolved src/language-js/utils.js Outdated
@j-f1

j-f1 approved these changes Oct 21, 2018

@j-f1

This comment has been minimized.

Member

j-f1 commented Oct 21, 2018

Looks like you’ve just got one lint error left.

@swac

This comment has been minimized.

Contributor

swac commented Oct 21, 2018

Thanks so much for reviewing! It looks like the commits in master are squashed, so I don't need to squash my commits in this branch, correct?

@j-f1

This comment has been minimized.

Member

j-f1 commented Oct 21, 2018

That’s correct! I’ll merge tomorrow if nobody has any objections.

@j-f1 j-f1 merged commit 4d4fab3 into prettier:master Oct 23, 2018

10 checks passed

ci/circleci: build_prod Your tests passed on CircleCI!
Details
ci/circleci: checkout_code Your tests passed on CircleCI!
Details
ci/circleci: test_prod_node4 Your tests passed on CircleCI!
Details
ci/circleci: test_prod_node9 Your tests passed on CircleCI!
Details
ci/circleci: test_prod_standalone Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 80%)
Details
codecov/project 96.39% (+<.01%) compared to d1d1f17
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

@swac swac deleted the swac:flowcomments branch Oct 23, 2018

@ikatyang ikatyang added this to the 1.15 milestone Oct 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment