Don't default parser to babylon by suchipi · Pull Request #4426 · prettier/prettier

suchipi · 2018-05-05T17:50:19Z

I couldn't figure out where to put the warning message; the inferParser logic is buried in normalizeOptions, but the logger is in the context 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).

suchipi · 2018-05-05T18:57:57Z

tests_integration/__tests__/debug-print-doc.js

+    ["--parser", "babylon", "--debug-print-doc"],
+    { input: "0" }
+  ).test({
+    stdout: '["0", ";", hardline, breakParent]',


I'm not sure why this changed, but the new output looks better than the old output, to me

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)

suchipi · 2018-05-05T18:58:47Z

tests/cursor/jsfmt.spec.js

 })`;
-  expect(prettier.formatWithCursor(code, { cursorOffset: 24 })).toEqual({
+  expect(
+    prettier.formatWithCursor(code, { parser: "babylon", cursorOffset: 24 })


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.

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

suchipi · 2018-05-05T18:59:44Z

src/language-raw/index.js

+    locStart() {
+      return 0;
+    },
+    locEnd(node) {


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.

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.

@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.

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.

I understand what they're for, I just didn't think they were required.

Looking at the docs again now, I totally misread them, my bad

suchipi · 2018-05-05T19:01:11Z

tests_integration/__tests__/__snapshots__/cursor-offset.js.snap

-"1;
-"
-`;
+exports[`write cursorOffset to stderr with --cursor-offset <int> (stdout) 1`] = `" 1"`;


Not sure why these changed... @ikatyang could you spot-check?

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|

duailibe · 2018-05-05T20:57:29Z

I'll see if I can figure out why the outputs changed later. In the meantime, can you add a test for formatWithCursor and when using the range feature (when using the raw parser)?

ikatyang · 2018-05-06T08:16:59Z

src/main/core-options.js

    type: "choice",
-    default: "babylon",
-    description: "Which parser to use.",
+    default: "raw",


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" }

],

ikatyang · 2018-05-06T08:55:39Z

I couldn't figure out where to put the warning message; the inferParser logic is buried in normalizeOptions, but the logger is in the context at the util-level. Could I get some help on that part?

I guess we can add a new field message or something to parser's choiceInfo and then show the warning in normalizeOptions based on that field:

parser: {
  choices: [
    { value: "raw", message: "blahblahblah" }
  ]
}

lydell · 2018-05-06T12:05:01Z

In #2884 (comment) I was more thinking about prettier foo.unknown not doing anything except printing a warning message. I had not a “raw” parser in mind. Do we need it?

suchipi · 2018-05-06T17:53:39Z

@lydell We don't need it, it was just the way I found to implement it.

suchipi · 2018-05-06T18:13:27Z

@ikatyang one problem with showing the warning in normalizeOptions is that we call it more than once during normal use, so the warning would get printed multiple times. I looked into implementing a hash map of files that we've already warned about but it felt really messy.

suchipi · 2018-05-15T20:16:41Z

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

suchipi · 2018-05-23T16:56:17Z

Closing; superceded by #4528

Don't default parser to babylon

26a3bde

suchipi added type:infra Issues about CI, publishing to npm, or similar area:cli Issues with Prettier's Command Line Interface status:wip Pull requests that are a work in progress and should not be merged labels May 5, 2018

suchipi added 2 commits May 5, 2018 11:58

Don't default to raw parser in the playground

deb3cf5

Fix tests

a613cfd

suchipi commented May 5, 2018

View reviewed changes

ikatyang reviewed May 6, 2018

View reviewed changes

suchipi added 2 commits May 6, 2018 12:02

Add cursor position and range tests for raw parser

329000c

Use since in parser option for support info

88c7626

suchipi mentioned this pull request May 6, 2018

Consider removing default parser / Skip processing unsupported file types #2884

Closed

suchipi added this to the 1.13 milestone May 9, 2018

lydell mentioned this pull request May 17, 2018

HTML comment throws SyntaxError #4497

Closed

duailibe mentioned this pull request May 23, 2018

Don't default parser to babylon #4528

Merged

suchipi closed this May 23, 2018

lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Aug 21, 2018

lock bot locked as resolved and limited conversation to collaborators Aug 21, 2018

	default: [
	{ since: "1.8.2", value: true },
	{ since: "1.9.0", value: "preserve" }
	],

Uh oh!

Conversation

suchipi commented May 5, 2018 • edited by ikatyang Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kachkaev May 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ikatyang May 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duailibe commented May 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ikatyang commented May 6, 2018

Uh oh!

lydell commented May 6, 2018

Uh oh!

suchipi commented May 6, 2018

Uh oh!

suchipi commented May 6, 2018

Uh oh!

suchipi commented May 15, 2018

Uh oh!

suchipi commented May 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

suchipi commented May 5, 2018 •

edited by ikatyang

Loading

kachkaev May 5, 2018 •

edited

Loading

ikatyang May 6, 2018 •

edited

Loading

duailibe commented May 5, 2018 •

edited

Loading