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

Conversation

@outofambit
Copy link
Contributor

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

@j-f1
j-f1 approved these changes Nov 20, 2018
Copy link
Member

left a comment

LGTM!

@j-f1 j-f1 requested a review from duailibe Nov 20, 2018

@j-f1 j-f1 merged commit e12cd17 into prettier:master Nov 23, 2018

10 checks passed

ci/circleci: build_prod Your tests passed on CircleCI!
Details
ci/circleci: checkout_code Your tests passed on CircleCI!
Details
ci/circleci: test_prod_node4 Your tests passed on CircleCI!
Details
ci/circleci: test_prod_node9 Your tests passed on CircleCI!
Details
ci/circleci: test_prod_standalone Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 80%)
Details
codecov/project 97.08% (+<.01%) compared to 6ee2f46
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
prettier.prettier Build #20181120.1 succeeded
Details
@j-f1

This comment has been minimized.

Copy link
Member

commented Nov 23, 2018

Thanks for contributing @outofambit!

@j-f1 j-f1 removed the request for review from duailibe Nov 23, 2018

@outofambit outofambit deleted the outofambit:list-different-and-write-exit-code branch Nov 23, 2018

@ikatyang ikatyang added this to the 1.15.3 milestone Nov 23, 2018

@felixfbecker

This comment has been minimized.

Copy link

commented Jan 11, 2019

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

This comment has been minimized.

Copy link

commented Jan 11, 2019

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

@j-f1

This comment has been minimized.

Copy link
Member

commented Jan 11, 2019

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

@felixfbecker

This comment has been minimized.

Copy link

commented Jan 11, 2019

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

This comment has been minimized.

Copy link
Member

commented Jan 11, 2019

Why are you using both in CI?

@felixfbecker

This comment has been minimized.

Copy link

commented Jan 11, 2019

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

This comment has been minimized.

Copy link
Member

commented Jan 11, 2019

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

@felixfbecker

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Member

commented Jan 11, 2019

cc @prettier/core

felixfbecker added a commit to sourcegraph/sourcegraph that referenced this pull request Jan 11, 2019
@outofambit

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2019

@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

This comment has been minimized.

Copy link

commented Jan 11, 2019

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 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.
Projects
None yet
5 participants
You can’t perform that action at this time.