-
-
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
Don't default parser to babylon #4426
Conversation
["--parser", "babylon", "--debug-print-doc"], | ||
{ input: "0" } | ||
).test({ | ||
stdout: '["0", ";", hardline, breakParent]', |
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'm not sure why this changed, but the new output looks better than the old output, to me
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 think maybe this printed content was being formatted with the babylon parser before (it was interpreting it as an ExpressionStatement
> ArrayExpression
and adding a semicolon)
@@ -30,7 +36,9 @@ test("doesn't insert second placeholder for nonexistent TypeAnnotation", () => { | |||
foo('bar', cb => { | |||
console.log('stuff') | |||
})`; | |||
expect(prettier.formatWithCursor(code, { cursorOffset: 24 })).toEqual({ | |||
expect( | |||
prettier.formatWithCursor(code, { parser: "babylon", cursorOffset: 24 }) |
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.
It's worth noting that you now must specify a parser when using the API, or your code will be passed through as-is. This might be counter-intuitive, and is a bit more of a breaking change than the CLI fix. We might want to default to babylon, but only when using the API directly.
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 think we can go back to #2884 and ask/ping if anyone's using the API without both filepath
and parser
and relying on the babylon fallback
locStart() { | ||
return 0; | ||
}, | ||
locEnd(node) { |
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 had to add locStart and locEnd to get the cursor-related tests passing; are these required on all parsers now? If so, we should update the documentation.
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.
It is documented: https://prettier.io/docs/en/plugins.html#parsers
I don't know if it was intentional. Since we need those for comments and other stuff, we just assume they are always present and call them. I guess we could do a check if they're present and disable those functionalities if they're not, but I don't know if it's worth the trouble.
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.
@duailibe it might. In https://github.com/gicentre/prettier-plugin-elm that I maintain locStart and locEnd make over 50% of code, but I have no idea what they are doing and how 😅 I just copied them from the python plugin and could not delete without breaking things.
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.
locStart
and locEnd
are supposed to take in a node
and return the index in the source code where said node starts or ends, respectively.
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 understand what they're for, I just didn't think they were required.
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.
Looking at the docs again now, I totally misread them, my bad
"1; | ||
" | ||
`; | ||
exports[`write cursorOffset to stderr with --cursor-offset <int> (stdout) 1`] = `" 1"`; |
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.
Not sure why these changed... @ikatyang could you spot-check?
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.
This looks like the correct change:
-
with
babylon
:before (offset=2)
1|
after (offset=1)
1|;
-
with
raw
:before (offset=2)
1|
after (offset=2)
1|
I'll see if I can figure out why the outputs changed later. In the meantime, can you add a test for |
src/main/core-options.js
Outdated
@@ -85,8 +85,9 @@ const options = { | |||
since: "0.0.10", | |||
category: CATEGORY_GLOBAL, | |||
type: "choice", | |||
default: "babylon", | |||
description: "Which parser to use.", | |||
default: "raw", |
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.
prettier/src/language-markdown/options.js
Lines 13 to 16 in 7e81a61
default: [ | |
{ since: "1.8.2", value: true }, | |
{ since: "1.9.0", value: "preserve" } | |
], |
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.
Will do
I guess we can add a new field parser: {
choices: [
{ value: "raw", message: "blahblahblah" }
]
} |
In #2884 (comment) I was more thinking about |
@lydell We don't need it, it was just the way I found to implement it. |
@ikatyang one problem with showing the warning in |
If someone has some time to figure out the logging part of this, I would greatly appreciate it. Otherwise, it'll get done when it gets done; still planning on it being in 1.13 though unless something comes up |
Closing; superceded by #4528 |
Fixes #2884
I couldn't figure out where to put the warning message; the
inferParser
logic is buried innormalizeOptions
, but the logger is in thecontext
at the util-level. Could I get some help on that part?Also, another consideration for the warning message is that it should be slightly different when they are using stdin (should probably mention using --stdin-filepath, etc).