-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
ensure --list-different + --write reports status code 0 #5512
ensure --list-different + --write reports status code 0 #5512
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.
LGTM!
Thanks for contributing @outofambit! |
This is a breaking change and should have been in a major version. We rely on |
There actually doesn't even seem to be any way to achieve this now because |
If you don’t pass |
Yey. I have more than 25 repositories with an npm script that does both |
Why are you using both in CI? |
Because I defined an npm script called |
You could set it up with just |
Yes, but I would have to define the glob twice, or have one script call into the other. I'm not saying it's not possible, I'm just saying this requires me to update all these repositories and is therefor clearly a breaking change that should not have been introduced in a patch release (and reverted, tbh). |
cc @prettier/core |
@felixfbecker there was some discussion of this in #4144. I believe we determined this to be a bug, so fixing it would not constitute a breaking change. |
The workaround I am using now is "prettier": "prettier --list-different --write \"**/{*.{js?(on),ts,md,yml},.*.yml}\"",
"prettier-check": "npm run prettier -- --write=false", |
closes #4144
let me know if i missed anything! 💖
addedmodified tests to confirm my change works.(If changing the API or CLI) I’ve documented the changes I’ve made (in thedocs/
directory)✨Try the playground for this PR✨