-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
fix & test handling of foo(, baz)
(first argument missing)
#10023
fix & test handling of foo(, baz)
(first argument missing)
#10023
Conversation
PR #9857 looks like a better approach, but it does not seem to cover function calls. |
I think this one is good enough before we decide what to do in #9979, can you fix this? Prettier pr-10023 --parser babel Input: a(,) Output: TypeError: Cannot read property 'type' of null
at isTemplateOnItsOwnLine (https://deploy-preview-10023--prettier.netlify.app/lib/standalone.js:15:162062)
at printCallExpression (https://deploy-preview-10023--prettier.netlify.app/lib/standalone.js:15:263948)
at https://deploy-preview-10023--prettier.netlify.app/lib/standalone.js:15:270263
at Object.print (https://deploy-preview-10023--prettier.netlify.app/lib/standalone.js:15:279870)
at Na (https://deploy-preview-10023--prettier.netlify.app/lib/standalone.js:15:115224)
at https://deploy-preview-10023--prettier.netlify.app/lib/standalone.js:15:114686
at printComments (https://deploy-preview-10023--prettier.netlify.app/lib/standalone.js:15:111129)
at e (https://deploy-preview-10023--prettier.netlify.app/lib/standalone.js:15:114677)
at ya.call (https://deploy-preview-10023--prettier.netlify.app/lib/standalone.js:15:112909)
at https://deploy-preview-10023--prettier.netlify.app/lib/standalone.js:15:268005 |
@@ -0,0 +1 @@ | |||
foo(, baz); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should test the cases with and without comments in each of the positions before and after the comma, for example:
foo(, baz); | |
foo(, baz); | |
foo(,); | |
foo(/*first*/, baz); | |
foo(,/*second*/ baz); | |
foo(, baz /* c */); | |
foo(/* first */, /* second */ baz /* c */); | |
foo(/* first */,); | |
foo(, /* second */); | |
foo(/* first */, /* second */); |
@fisker I may need a few days due to some urgent work, would not mind if you want to take this one over. I wonder if we should test with and without comments like I suggested on the test code. Thanks for looking into this. |
@@ -0,0 +1 @@ | |||
run_spec(__dirname, ["babel-flow", "babel-ts"]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I tried with "babel" I did encounter what I think was a parse error. I think it would be ideal if we could test with ["babel", "babel-ts"]
or event ["babel", "typescript"]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two cases not good:
Prettier pr-10023
Playground link
--parser babel
Input:
foo(/*bar*/, baz)
Output:
foo /*bar*/(, baz);
Prettier pr-10023
Playground link
--parser babel
Input:
foo(/*bar*/, /*baz*/)
Output:
TypeError: Cannot read property 'type' of null
at isTemplateOnItsOwnLine (https://deploy-preview-10023--prettier.netlify.app/lib/standalone.js:15:162062)
at printCallExpression (https://deploy-preview-10023--prettier.netlify.app/lib/standalone.js:15:263948)
at https://deploy-preview-10023--prettier.netlify.app/lib/standalone.js:15:270263
at Object.print (https://deploy-preview-10023--prettier.netlify.app/lib/standalone.js:15:279870)
at Na (https://deploy-preview-10023--prettier.netlify.app/lib/standalone.js:15:115224)
at https://deploy-preview-10023--prettier.netlify.app/lib/standalone.js:15:114686
at printComments (https://deploy-preview-10023--prettier.netlify.app/lib/standalone.js:15:111129)
at e (https://deploy-preview-10023--prettier.netlify.app/lib/standalone.js:15:114677)
at ya.call (https://deploy-preview-10023--prettier.netlify.app/lib/standalone.js:15:112909)
at https://deploy-preview-10023--prettier.netlify.app/lib/standalone.js:15:268005
Closing this as Prettier doesn't try to print code with omitted call arguments anymore. See #10065. |
Description
from this review comment on PR #9992: #9992 (comment)
Without these changes, code like
foo(, baz)
would lead to an ugly exception withbabel-flow
&babel-ts
parsers./cc @fisker
Checklist
(If changing the API or CLI) I’ve documented the changes I’ve made (in thedocs/
directory).(If the change is user-facing) I’ve added my changes tochangelog_unreleased/*/XXXX.md
file followingchangelog_unreleased/TEMPLATE.md
.✨Try the playground for this PR✨