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 to provide a format command #4374

Closed
wants to merge 1 commit into from
Closed

Conversation

noisysocks
Copy link
Member

@noisysocks noisysocks commented Jan 10, 2018

See #2819.

Uses prettier-eslint to add a format command to Gutenberg. This lets a developer easily re-format a JavaScript file to meet our existing linter rules.

Demo:

asciicast

Testing:

Check out this branch, npm install, then run:

$ npm run format -- path/to/a/file.js

Notes:

@noisysocks
Copy link
Member Author

cc. @gziolo

@gziolo gziolo requested review from aduth and youknowriad January 10, 2018 11:12
@gziolo
Copy link
Member

gziolo commented Jan 10, 2018

Demo looks cool. I will give it a try later this week 👍

@gziolo gziolo self-requested a review January 10, 2018 11:13
@gziolo gziolo added the [Type] Build Tooling Issues or PRs related to build tooling label Jan 10, 2018
@gziolo gziolo requested a review from ntwb January 10, 2018 11:14
@aduth
Copy link
Member

aduth commented Jan 10, 2018

Demo is cool indeed. I worry in general usage this might introduce many undesirable (or at least unrelated) changes to existing files when applied. I also don't have strong confidence that ESLint's --fix is yet able to force all styles to our standard (though it has improved significantly over the past while).

Is this something we need to have built-in tooling for, or could we just encourage developers to install these packages globally and at worst create an alias to run the command?

Uses prettier-eslint to add a 'format' script which automatically
formats a JavaScript file according to our lint rules.

Usage:

npm run format -- path/to/file.js
@gziolo
Copy link
Member

gziolo commented Jan 11, 2018

Demo is cool indeed. I worry in general usage this might introduce many undesirable (or at least unrelated) changes to existing files when applied. I also don't have strong confidence that ESLint's --fix is yet able to force all styles to our standard (though it has improved significantly over the past while).

We can follow the approach Calypso devs took. They added prettier setup to the repository without any integration provided. This enabled transition period where everyone could experiment on their own. This would allow to verify if --fix is reliable enough to use it on a larger scale. If there would be any issues we can always try calypso-prettier fork which is solid enough to consume it in a larger project and respects WordPress coding styles.

Is this something we need to have built-in tooling for, or could we just encourage developers to install these packages globally and at worst create an alias to run the command?

@aduth does it mean you rather suggest using npx instead like this:

"format": "npx prettier-eslint-cli@4.7.0 -p prettier-eslint@8.5.0 --tab-width 4 --write",

I haven't executed it, but it should work in one of the variations with the -p flag positioned properly :)

@aduth
Copy link
Member

aduth commented Jan 11, 2018

@aduth does it mean you rather suggest using npx instead like this:

I more meant not having any scripts in Gutenberg proper, just encouraging developers to run on their own machines:

prettier-eslint --tab-width 4 --write

I don't feel particularly strongly about this.

@gziolo
Copy link
Member

gziolo commented Jan 11, 2018

I'm okey with having it exposed inside Gutenberg. If there are more people sharing this concern we can expose it indirectly with the @wordpress/scripts package I started working on with this PR: WordPress/packages#62. You would then use it as ./node_modules/.bin/wp-scripts format.

@noisysocks
Copy link
Member Author

noisysocks commented Jan 11, 2018

I definitely don't think we're at a stage where it's sensible to enforce this or to run it automatically.

Still, I think there's value in having this command set up in scripts (or @wordpress/scripts) with some documentation in the README so that there's a very low barrier of entry into this useful (in my opinion) tooling.

@ntwb
Copy link
Member

ntwb commented Jan 22, 2018

Related: #4628

@ahmadawais
Copy link
Contributor

Thanks for the PR @noisysocks but I don't think this is the right way to integrate Prettier. I shared my thoughts over at a similar PR by @ntwb #4628 — in an initial PR review here.

Check it out, let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Build Tooling Issues or PRs related to build tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants