Skip to content

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

Merged
j-f1 merged 2 commits intoprettier:masterfrom
outofambit:list-different-and-write-exit-code
Nov 23, 2018
Merged

ensure --list-different + --write reports status code 0#5512
j-f1 merged 2 commits intoprettier:masterfrom
outofambit:list-different-and-write-exit-code

Conversation

@outofambit
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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
Copy Markdown

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

@j-f1
Copy link
Copy Markdown
Member

j-f1 commented Jan 11, 2019

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

@felixfbecker
Copy link
Copy Markdown

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
Copy Markdown
Member

j-f1 commented Jan 11, 2019

Why are you using both in CI?

@felixfbecker
Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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