Skip to content

Don't default parser to babylon #4528

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.

Sign up for GitHub

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

Jump to bottom
Merged
duailibe merged 8 commits into from
May 23, 2018
Merged

Don't default parser to babylon #4528

duailibe merged 8 commits into from
May 23, 2018

Conversation

@duailibe
Copy link
Collaborator

@duailibe duailibe commented May 23, 2018

Closes #2884

This takes a different approach from #4426 to make it more explicit a parser wasn't specified or inferred (returns normalized opts as null and format() bails formatting).

lydell
lydell reviewed May 23, 2018
View reviewed changes
src/main/options.js Outdated
if (!rawOptions.parser) {
const logger = (opts && opts.logger) || console;
if (!rawOptions.filepath) {
logger.warn("Skipping file with unknown filepath");
Copy link
Member

@lydell lydell May 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is an unknown filepath?

Copy link
Collaborator Author

@duailibe duailibe May 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lydell Use stdin without --stdin-filepath or prettier.format without { filepath }

Copy link
Collaborator Author

@duailibe duailibe May 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first I decided to throw an error but I figured that could be a little too harsh

Copy link
Member

@lydell lydell May 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I wanted to come up with a better message, but it's hard.

Copy link
Member

@lydell lydell May 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really like the updated message, good job!

@lydell
Copy link
Member

lydell commented May 23, 2018 •
edited
Loading

I fetched your branch and played around with it a little, and it seems nice.

Minor logging glitch:

❯ ./bin/prettier.js LICENSE index.js --write
LICENSESkipping file with unknown extension: LICENSE
LICENSE 2ms
index.js 59ms

Question about stdout:

❯ ./bin/prettier.js LICENSE

Currently the above print LICENSE as-is to stdout. Is it better to not print it to stdout at all? Similar: What should prettier.format("foo") return?

suchipi
suchipi reviewed May 23, 2018
View reviewed changes
src/main/core-options.js Outdated
category: CATEGORY_GLOBAL,
type: "choice",
default: "babylon",
default: undefined,
Copy link
Member

@suchipi suchipi May 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the support info, you need to specify that the default varies across versions; see #4426 (comment)

@suchipi suchipi mentioned this pull request May 23, 2018
Closed
suchipi
suchipi approved these changes May 23, 2018
View reviewed changes
Copy link
Member

@suchipi suchipi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, aside from the logging issue @lydell mentioned

@duailibe
Copy link
Collaborator Author

duailibe commented May 23, 2018

Currently the above print LICENSE as-is to stdout. Is it better to not print it to stdout at all? Similar: What should prettier.format("foo") return?

Good questions, I'm not sure. I'm comfortable with throwing an error in the latter, but I'm not sure what we should do about prettier LICENSE

@duailibe
Copy link
Collaborator Author

duailibe commented May 23, 2018 •
edited
Loading

So I tried to improve the logging experience a bit but there are too much cases to cover 😅

This code is pretty complex, I'd appreciate more testing

@duailibe
Copy link
Collaborator Author

duailibe commented May 23, 2018 •
edited
Loading

I think I covered most use cases in infer-parser.js; should be easier to reason about the behavior now

duailibe
duailibe commented May 23, 2018
View reviewed changes
tests_integration/__tests__/infer-parser.js
});
});

describe("--write and --list-different with unknown path and no parser", () => {
Copy link
Collaborator Author

@duailibe duailibe May 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea what this means (how it should behave).. we really need to revamp the CLI

@duailibe duailibe changed the title Don't default parser to babylon (2) Don't default parser to babylon May 23, 2018
lydell
lydell approved these changes May 23, 2018
View reviewed changes
Copy link
Member

@lydell lydell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is so good! 😍

@duailibe duailibe merged commit 5c6e080 into prettier:master May 23, 2018
@duailibe duailibe deleted the default-parser branch May 23, 2018 19:55
@suchipi
Copy link
Member

suchipi commented May 23, 2018

🎉

This was referenced May 24, 2018
Merged
Merged
This was referenced May 28, 2018
Closed
Merged
@ikatyang ikatyang mentioned this pull request May 30, 2018
Merged
@uxuf uxuf mentioned this pull request Jun 4, 2018
Closed
@duailibe duailibe mentioned this pull request Jun 15, 2018
Closed
@ikatyang ikatyang mentioned this pull request Jun 17, 2018
Closed
@johanhenrikssn johanhenrikssn mentioned this pull request Jul 13, 2018
Merged
@dhruvdutt dhruvdutt mentioned this pull request Jul 19, 2018
Merged
dhruvdutt added a commit to webpack/webpack-cli that referenced this pull request Jul 21, 2018
@dhruvdutt
fix(prettier): add parser, filePath (#553)
acf0fd9 
**What kind of change does this PR introduce?**
Bugfix

**Did you add tests for your changes?**
No

**If relevant, did you update the documentation?**
No

**Summary**
![image](https://user-images.githubusercontent.com/5961873/42949683-ef845284-8b8f-11e8-9f3a-d8d4c673a542.png)

Prettier shows warning currently that filePath and parser aren't specified.
It would throw errors in future major versions. [#4828](prettier/prettier#4528)

This PR cleans the warning shown in the above image.

<!-- Explain the **motivation** for making this change. What existing problem does the pull request solve? -->
<!-- Try to link to an open issue for more information. -->

**Does this PR introduce a breaking change?**
No

**Other information**
No
@lock 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 lock bot locked as resolved and limited conversation to collaborators Aug 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@suchipi suchipi suchipi approved these changes

@lydell lydell lydell approved these changes

Assignees

No one assigned

Labels

locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@duailibe @lydell @suchipi