Skip to content
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

Use Prettier Config API #11980

Merged
merged 1 commit into from
Jan 7, 2018
Merged

Use Prettier Config API #11980

merged 1 commit into from
Jan 7, 2018

Conversation

azz
Copy link
Contributor

@azz azz commented Jan 7, 2018

Currently when a developer is using a text editor Prettier plugin such as prettier-vscode or prettier-atom, the formatting functionality will format the files with incorrect settings (such as double quotes and no trailing commas). This is easily fixed by moving a lot of the logic in scripts/prettier/index.js upstream to Prettier via a configuration (.prettierrc.js) file.

I've used scripts/shared/pathsByLanguageVersion to maintain the addition of the trailingComma: "all" setting for ESNext files.

All four "modes" (check, check-changed, write, write-changed) have been maintained.

I have tested this by enabling format-on-save in prettier-vscode and saving both ES5 and ESNext files to check if the correct settings are applied. I have also tested the check-changed and write-changed files apply the correct rules to the two different types of files.

@@ -7,98 +7,67 @@
'use strict';

// Based on similar script in Jest
// https://github.com/facebook/jest/blob/master/scripts/prettier.js
// https://github.com/facebook/jest/blob/a7acc5ae519613647ff2c253dd21933d6f94b47f/scripts/prettier.js
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file has since been removed from Jest.

console.log(file);
files.forEach(file => {
const options = prettier.resolveConfig.sync(file, {
config: prettierConfigPath,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Passing in the path explicitly avoids looking up the FS tree for each file.

return;
}
if (!files.length) {
return;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are we ok with top-level return? I could change it to process.exit().

@blling
Copy link

blling commented Jan 7, 2018

Most of the time i use eslint for checking purpose, i think this is does`t matter :)

@azz
Copy link
Contributor Author

azz commented Jan 7, 2018

@blling This repository doesn't use eslint-plugin-prettier.

@blling
Copy link

blling commented Jan 7, 2018

@azz I am sorry, i mean in react, there is an exactly step npm run prettier for code prettier, and do not need other plugins. We could just use eslint(without eslint-plugin-prettier is OK) for necessary code inspection. This is my opinion :)

@azz
Copy link
Contributor Author

azz commented Jan 7, 2018

@blling I'm not sure how ESLint is relevant to the PR. This PR just fixes the default behaviour of Prettier editor integrations, which many people use to format code on save.

@blling
Copy link

blling commented Jan 7, 2018

@azz En, Some of the ESLint rule is the same as Prettier rule, eg: --no-semi, --single-quote and so on. Perhaps not all rules are the same but enough, am i right?

@azz
Copy link
Contributor Author

azz commented Jan 7, 2018

am i right?

No, Prettier is a fundamentally different tool to ESLint. ESLint is used to enforce particular sets of rules based on static analysis. It just so happens that some of those rules have localized fixes. Prettier on the other hand formats the entire file from scratch, and has some options to change the output slightly. Using Prettier when you save a file means that you don't have to worry about formatting your code manually at all almost. ESLint doesn't offer that.

There's some more info in the Prettier docs: https://prettier.io/docs/en/comparison

@blling
Copy link

blling commented Jan 7, 2018

@azz Thank you for your patiently explain, i will track the react team's response.

BTW:
I happened to be integrating Prettier for react-boilerplate in this PR, if you could give me some suggestions that will be nice:)

@gaearon gaearon merged commit 052a5f2 into facebook:master Jan 7, 2018
@gaearon
Copy link
Collaborator

gaearon commented Jan 7, 2018

This is awesome, thank you!

@azz azz deleted the prettier-config branch January 7, 2018 11:57
ManasJayanth pushed a commit to ManasJayanth/react that referenced this pull request Jan 12, 2018
@desasaliho
Copy link

Update HTTP to HTTPs in CHANGELOG.md (

@desasaliho
Copy link

Thanks you account disabled soon as my facebook

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

Successfully merging this pull request may close these issues.

None yet

5 participants