-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
add prettier and prettying everything #3401
Conversation
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.
Awesome!
scripts/prettier.js
Outdated
ignore: ['**/node_modules/**'], | ||
options: { | ||
'bracket-spacing': 'false', | ||
'print-width': 120, |
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.
What happens when we change this to 80 (or 100)? Prettiers output actually looks worse the longer the line-length is because it tries to cram everything into one line.
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.
ah ok I can reduce the number and run yarn prettier
if you prefer
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.
in the jest project there is 80 for print-width
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.
What's the point of scripts/prettier.js
? You can use eslint-plugin-prettify
and then you can use eslint . --fix
. Reusing the existing lint infra seems best.
Ah ok I really don't know that plugin, I can switch to eslint-plugin-prettify if we prefer, I see what the jest project does and copy it (infact if you see the file is the same apart some modifications). |
scripts/prettier.js
Outdated
@@ -0,0 +1,60 @@ | |||
/** |
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.
People say you can use thus plugin https://github.com/zertosh/eslint-plugin-prettify instead of this file now
@kittens @bestander @cpojer now should works as expected :) |
I am sorry but this not works as expected for some reasons:
I will dig further and maybe i will open another pull request. |
Thanks, should we revert? |
Or a patch would be fine? |
Maybe now it is better to revert everything, I really don't know how much it could take. |
@bestander after thinking a while, if i open a patch and I delete eslint-plugin-prettify and reintroduce the custom scripts, would it be fine? I mean without reverting everything |
Yeah, let's have a temporary patch |
add prettier and prettying everything (yarnpkg#3401)
@voxsim |
Summary
This fix #3318.
Test plan
Tests already in place and green :D