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

ensure --list-different + --write reports status code 0 #5512

Merged
merged 2 commits into from
Nov 23, 2018
Merged

ensure --list-different + --write reports status code 0 #5512

merged 2 commits into from
Nov 23, 2018

Conversation

outofambit
Copy link
Contributor

@outofambit outofambit commented Nov 20, 2018

closes #4144

let me know if i missed anything! 💖

  • I’ve added modified tests to confirm my change works.
  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory)
  • I’ve read the contributing guidelines.

Try the playground for this PR

Copy link
Member

@j-f1 j-f1 left a comment

Choose a reason for hiding this comment

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

LGTM!

@j-f1 j-f1 requested a review from duailibe November 20, 2018 21:29
@j-f1 j-f1 merged commit e12cd17 into prettier:master Nov 23, 2018
@j-f1
Copy link
Member

j-f1 commented Nov 23, 2018

Thanks for contributing @outofambit!

@j-f1 j-f1 removed the request for review from duailibe November 23, 2018 15:42
@outofambit outofambit deleted the list-different-and-write-exit-code branch November 23, 2018 15:51
@ikatyang ikatyang added this to the 1.15.3 milestone Nov 23, 2018
@felixfbecker
Copy link

This is a breaking change and should have been in a major version. We rely on --list-different to check files are formatted in CI and break the build if they are not. There is no mention in the changelog of what different flags need to be provided to maintain this behaviour. I noticed this because files would constantly get reformatted despite us checking in CI with --list-different.

@felixfbecker
Copy link

There actually doesn't even seem to be any way to achieve this now because --check is not released yet

@j-f1
Copy link
Member

j-f1 commented Jan 11, 2019

If you don’t pass --write, Prettier will still exit with 1.

@felixfbecker
Copy link

Yey. I have more than 25 repositories with an npm script that does both --write and --list-different and is used in CI.

@j-f1
Copy link
Member

j-f1 commented Jan 11, 2019

Why are you using both in CI?

@felixfbecker
Copy link

Because I defined an npm script called prettier that can be used both in CI and locally, to have a single point where the file pattern is defined.

See https://sourcegraph.com/search?q=r:%5Egithub.com/sourcegraph/+%22prettier%5C%22:+%5C%22prettier%22+f:package.json

@j-f1
Copy link
Member

j-f1 commented Jan 11, 2019

You could set it up with just --list-different and have a format script that runs yarn prettier --write.

@felixfbecker
Copy link

felixfbecker commented Jan 11, 2019

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).

@j-f1
Copy link
Member

j-f1 commented Jan 11, 2019

cc @prettier/core

felixfbecker added a commit to sourcegraph/sourcegraph-public-snapshot that referenced this pull request Jan 11, 2019
@outofambit
Copy link
Contributor Author

@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.

@felixfbecker
Copy link

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",

@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Apr 11, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Apr 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI: --list-different + --write = Exit status 1 if something was modified
5 participants