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

Add diff in output of --check (for CI use cases) #6885

Open
muescha opened this issue Nov 8, 2019 · 98 comments
Open

Add diff in output of --check (for CI use cases) #6885

muescha opened this issue Nov 8, 2019 · 98 comments
Labels
area:cli Issues with Prettier's Command Line Interface status:needs discussion Issues needing discussion and a decision to be made before action can be taken

Comments

@muescha
Copy link

muescha commented Nov 8, 2019

More people submit PR for documents in Markdown files or other things via Github web editor. If they make a mistake then the CI fails but there is no chance to inform the user whats wrong.

I see some infos should be with the --debug-check but currently there is no output with this option.

Environments:

  • Prettier Version: 1.18.2
  • Running Prettier via: cli
  • Runtime: node?
  • Operating System: linux

Steps to reproduce:

prettier "**/*.{md,css,scss,yaml,yml,ts}" "--check"

Expected behavior:

Show each error and the changes like with --debug-check here in #4415

Actual behavior:

Show nothing about the errors:

> @ prettier /home/circleci/project
> prettier "**/*.{md,css,scss,yaml,yml,ts}" "--check"

Checking formatting...
docs/contributing/environment.md
Code style issues found in the above file(s). Forgot to run Prettier?

Also there are no more infos available with this commands:

prettier "**/*.{md,css,scss,yaml,yml,ts}" "--debug-check"
prettier "**/*.{md,css,scss,yaml,yml,ts}" "--debug-check" "--check"
@dylannz-sailthru
Copy link

"Forgot to run Prettier?" is really not a very helpful message...

@thorn0
Copy link
Member

thorn0 commented Nov 15, 2019

The message contains the file names. What else do you need to run Prettier on those files? ¯\_(ツ)_/¯

@lydell
Copy link
Member

lydell commented Nov 15, 2019

I personally like the output of --check very much.

CI: "Forgot to run Prettier?"

Me: "Oh yeah, I guess I did!" Runs Prettier.

CI: "All good!"

@muescha
Copy link
Author

muescha commented Nov 15, 2019

I like to point again to:

more people submit PR for documents in Markdown files or other things via Github web editor.

That means they have no local IDE installed. That's why they can not run prettier on the file.

@muescha
Copy link
Author

muescha commented Nov 15, 2019

Expected behavior would be a detailed error message or a diff

[error] src/index.js: ast(input) !== ast(prettier(input))
[error] Index:
[error] ===================================================================
[error] ---
[error] +++
[error] @@ -17,6 +17,6 @@
[error]                    "method": false,
[error]                    "key": {
[error] -                    "type": "StringLiteral",
[error] -                    "value": "asd"
[error] +                    "type": "Identifier",
[error] +                    "name": "asd"
[error]                    },
[error]                    "computed": false,
[error]
[error] Index:
[error] ===================================================================
[error] ---
[error] +++
[error] @@ -1,1 +1,1 @@
[error] -const test = { "asd": 1 };
[error] +const test = { asd: 1 };
[error]

@alexander-akait
Copy link
Member

alexander-akait commented Nov 15, 2019

Why not run prettier \"{**/*,*}.{js,json,md,yml,css}\" --list-different" using npm/yarn script?

@alexander-akait alexander-akait added status:awaiting response Issues that require answers to questions from maintainers before action can be taken type:question Questions and support requests. Please use Stack Overflow for them, not the issue tracker. labels Nov 15, 2019
@muescha
Copy link
Author

muescha commented Nov 16, 2019

thx. I would try on monday the output of --list-different

@no-response no-response bot removed the status:awaiting response Issues that require answers to questions from maintainers before action can be taken label Nov 16, 2019
@lydell
Copy link
Member

lydell commented Nov 16, 2019

--list-different also just lists file names – I don’t think it’ll make any difference for @muescha.

@muescha I think this proposal might be solving your problem the wrong way. Forcing people to click into a CI build log, find diffs of expected output and then manually applying them does no sound very friendly. I think we could try several other things:

  • Encourage people to clone the project rather than using the web editor.
  • Use --prose-wrap preserve or --prose-wrap never to reduce chance of needing to run Prettier.
  • Running Prettier in the web editor via https://github.com/prettier/prettier-chrome-extension.
  • A bot that automatically runs Prettier on PRs.
  • You could pull people’s branches, run Prettier yourself and push to their PR (contributions from maintainers is generally enabled).
  • Not use Prettier for markdown files if you get a lot of contributions to them via the web editor.

@lydell lydell added the status:awaiting response Issues that require answers to questions from maintainers before action can be taken label Nov 16, 2019
@muescha
Copy link
Author

muescha commented Nov 23, 2019

  • Encourage people to clone the project rather than using the web editor.

thats much overhead :(

  • Use --prose-wrap preserve or --prose-wrap never to reduce chance of needing to run Prettier.

it should run normal prettier

  • Running Prettier in the web editor via prettier/prettier-chrome-extension.

this is only the solution for one user, and not for all

  • A bot that automatically runs Prettier on PRs.

this changes the PR - the user should have information about the problems

  • You could pull people’s branches, run Prettier yourself and push to their PR (contributions from maintainers is generally enabled).

to much overhead to checkout a branch etc

  • Not use Prettier for markdown files if you get a lot of contributions to them via the web editor.

also prettier is a good help

the only thing is that there should be an more verbose error message :/

@no-response no-response bot removed the status:awaiting response Issues that require answers to questions from maintainers before action can be taken label Nov 23, 2019
@lydell
Copy link
Member

lydell commented Nov 25, 2019

Thanks for the responses!

You didn’t seem to respond to this one, though:

Forcing people to click into a CI build log, find diffs of expected output and then manually applying them does no sound very friendly.

Thoughts?

@lydell lydell added the status:awaiting response Issues that require answers to questions from maintainers before action can be taken label Nov 25, 2019
@muescha
Copy link
Author

muescha commented Nov 27, 2019

since they edit the PR on github and check why the CI build is failing they already looking into the CI logs. and then when they only find a not helpful message - that is frustrating.

example gatsbyjs/gatsby#18719 (comment)

I checked what you suggested and lines where only word to highlight is "false" (without quotes or 0 are highlighted with no issues.

PS. @muescha is there a way to get rid of prettier complains in build errors? I am editing this in the browser.

@no-response no-response bot removed the status:awaiting response Issues that require answers to questions from maintainers before action can be taken label Nov 27, 2019
@dylannz-sailthru
Copy link

dylannz-sailthru commented Nov 27, 2019

I was making changes to a repository that'd had prettier added to it without me realising, but I had not installed it in my local development environment. So naturally it blew up in CI and asked if I'd "forgotten to run prettier". But that message came from a program called "prettier", which was pretty confusing - yes I suppose I'd "forgotten" to run prettier (I hadn't actually, I didn't even know I had to. Unnecessarily snarky error message, if you ask me...) but even more annoying was that the prettier program was printing that message!

exit 1 would have been just as helpful, and less condescending. A list of formatting violations would have been a lot better though!

@thorn0
Copy link
Member

thorn0 commented Nov 27, 2019

@dylannz-sailthru Would you really prefer to get a diff instead like @muescha has proposed and apply it manually instead of running the formatter?

@dylannz-sailthru
Copy link

@dylannz-sailthru Would you really prefer to get a diff instead like @muescha has proposed and apply it manually instead of running the formatter?

I admit, I've now set up my IDE with prettier and now it doesn't bother me. But it seems there is a use case where someone wants to make a small change and doesn't have prettier installed (either on GitHub or in a local development environment), adds an extra space somewhere, and now has to install a 3rd party program and run it instead of just fixing a small error. And while I'll admit it's a narrow use case, I can't see much of a downside to being more verbose (or at least having a configurable verbose flag). Most CI tools will collapse each CI step so you can view the logs of each one in isolation.

@muescha
Copy link
Author

muescha commented Nov 27, 2019

example where i used the GitHub editor and i get for this change gatsbyjs/gatsby@7b78781 in gatsbyjs/gatsby#19822 this CI log message:

https://circleci.com/gh/gatsbyjs/gatsby/283065?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

now i have to checkout ~500MB data from pull request, install prettier only just to see whats wrong in this commit and i can change the file?

@thorn0
Copy link
Member

thorn0 commented Nov 27, 2019

@dylannz-sailthru Locally, extra setup isn't needed. Prettier is installed as a dev dependency and there is usually a command in the project to run all the linters, formatters, etc. (e.g. npm run format). This means we're discussing only the web UI use case here.

@lydell
Copy link
Member

lydell commented Nov 27, 2019

Unnecessarily snarky error message, if you ask me...

Sorry, the message is actually not supposed to be snarky, but friendly! Do you have ideas for better wording?

example gatsbyjs/gatsby#18719 (comment)

I don’t think that’s a good example. That’s not a markdown change, but a code change! Is that really something people do in the browser? That sounds terrible to me – how do you debug and test then?


Here’s yet one idea: When editing a markdown file in the GitHub editor and are about to hit the Commit button – first copy-paste the entire file into https://prettier.io/playground/ and then copy back the output.

@muescha
Copy link
Author

muescha commented Nov 27, 2019

I don’t think that’s a good example. That’s not a markdown change, but a code change! Is that really something people do in the browser?

yes - people do it. so why not also on all files.

Here’s yet one idea

that maybe a workaround but not a solution and also not fit with the project settings for prettier

@dylannz-sailthru
Copy link

Sorry, the message is actually not supposed to be snarky, but friendly! Do you have ideas for better wording?

Sure! Something like: You need to run `prettier fix` on these files to ensure they are formatted correctly.

This way it's telling me more specifically what I need to do to get past the error. I think it doesn't matter so much for people who have used prettier before, but as someone who hadn't used it before I found it a little confusing.

Here’s yet one idea: When editing a markdown file in the GitHub editor and are about to hit the Commit button – first copy-paste the entire file into https://prettier.io/playground/ and then copy back the output.

Actually that'd be a great thing to link to in the error message as well - I'd have found it really useful at the time :)

@asfaltboy

This comment has been minimized.

@lydell lydell changed the title Show Detailed Infos on CI linke circleCI Show Detailed Info on CI like CircleCI Aug 1, 2020
@grncdr
Copy link

grncdr commented Sep 8, 2020

I would also like to see a diff or more detailed information about why prettier thinks a file isn't formatted. I currently don't run prettier --check in CI/pre-commit hooks because it tells me that files I've just formatted with Prettier are still not formatted. I really don't know how to begin fixing the problem when I can't see what it is.

@lydell
Copy link
Member

lydell commented Sep 8, 2020

@grncdr It tells you which files aren’t formatted, so all you need to do is run Prettier on those files. Or on your whole project – probably prettier --write .. Can you expand on how seeing a diff helps?

@daveisfera
Copy link

...many CI tools already parse it...

@daveisfera Could you name a few?

GitHub Actions and Bamboo are the two that I know of that can parse this output

@Jasperrr91
Copy link

@lydell

  • Instead of carefully inspecting the diff of prettier --check before running prettier --write, you could carefully inspect the diff of git diff after prettier --write.

I understand where you are coming from but prettier --write is fairly intrusive in that it overwrites all of your files. If users had forgotten to stage/commit files beforehand (and that happens often enough) they cannot simply discard the changes caused by a incorrect configured prettier.

Of course, one could edit their prettierconfig and run it again and again till the desired config is reached but still, there's no way to properly revert.

I see a diff in --check more of as an UX improvement for CI, not something you’d run all the time. You’re of course free to use --check however you like, but I think there are better ways to use Prettier!

There might indeed be better ways to use Prettier but looking at the amount of comments and votes in this thread, there is clearly a lot of people that would like to use Prettier in this specific way. Now, it's easy to support both ways of working. Simply add a --verbose flag so that users that want more detailed logs can run prettier --check --verbose while others can stick to the regular prettier --check.

I also agree that the current error message is incorrect either way. "Forgot to run Prettier" is a weird error to give to a user that has just ran Prettier.

@aduh95
Copy link

aduh95 commented Mar 17, 2023

As a workaround, if your project is using git, you can use prettier . -c || (git diff --quiet || (echo 'Unable to show a diff because there are unstaged changes'; false) && (prettier . -w --loglevel silent && git --no-pager diff; git restore .) && false) to generate a diff. It would still be worth if Prettier could generate a diff itself of course, but the above command can already improve the DX.

@shasherazi
Copy link

Still no diffs in outputs???

@jiri-thiemel-te
Copy link

Listing the rule(s) because of which a file contains warning(s) is necessary to understand the current setup. (E.g. when some behaviour makes you crazy and you want to find out which rule is responsible for that to modify it). I'M SHOCKED that there is no way to list this when running prettier command. How some functionality as basic and expected as this one can be missing is beyond me.

@alexey-sh
Copy link

alexey-sh commented May 2, 2023

Is there any chance to have a helpful message while --check run?
It seems like 4 years isn't enough for that. I hope in 2030 it will be possible

@abhijeetviswa
Copy link

I agree with others that this is some pretty basic functionality that is provided by tooling in other languages (isort, black in Python). At the very least, it'd be useful to know as a beginner to the codebase what you messed up when it comes to the style so that you can make a conscious effort at not repeating it instead of relying on the IDE to magically fix things for you.

For me style checks in a CI env is akin to UTs. Think of how annoying and frustrating it would be if your test suite said "A test failed in your project. Did you not write it properly?" without giving you ANY further logs on which test failed.

@mukeshsharma1201
Copy link

I encountered this issue today!
I am a backend dev and needed to do a quick bug fix in the frontend codebase.
I made one line change and pushed the code to find that the CI is failing with the message Code style issues found in the above file. Forgot to run Prettier?

I don't want to install whole npm and frontend packages on my machine just to find out 1 small issue that I could have fixed right away if there was a simple diff or description about the issue!

And this issue is Open for 4 years now!!

@maxolasersquad
Copy link

maxolasersquad commented Jun 23, 2023

Agreed. Just saying "something" failed on the failed, without saying what is not as useful as it should be. For example, eslint gives output like this.

/home/foo/projects/src/components/Foo.js
  33:1  error  `FooInput` import should occur before import of `HamburgerInput`  import/order

✖ 1 problem (1 error, 0 warnings)
  1 error and 0 warnings potentially fixable with the `--fix` option.

Not only does it tell me which file had a failure, but it also tells me at what line, and what the failure is. If I'm confused, I can search the web for "eslint import/order" for additional information. A similar output from prettier would be very helpful.

My current method of understanding prettier failures is to run with the --write option, and then looking at the git diff, which is not preferable.

@ngerritsen
Copy link

ngerritsen commented Aug 2, 2023

So we have our Prettier setup correctly with our developers IDE's, but now we are running into some issues in the CI pipeline. It's probably some cache issue, also probably unrelated to Prettier itself.

But anyway, whatever the actual issue is, it would be quite helpful if we could see what the problem in a file is in the CI logs, because on our local IDE's/terminals we don't have the issue. It would make debugging why our CI is failing much faster, we could see if it's maybe running it on an old version of the file or whatever.

I think it's a pretty mundane feature that most linters have and I think it makes sense for Prettier too. I know Prettier is not really a linter, but still, it's a script that runs automatic checks on your code.

@webark
Copy link
Contributor

webark commented Aug 2, 2023

Have you tried the "Prettier write -> git diff" method?

@maxolasersquad
Copy link

The "Prettier write -> git diff" method doesn't really work in CI pipelines. The container running the pipeline probably doesn't even have git installed.

@webark
Copy link
Contributor

webark commented Aug 2, 2023

I mean.. Depends on the pipelines. There are a lot of pipelines that do some validation, and then generate a git tag and push it.

@george-gca
Copy link

george-gca commented Jan 16, 2024

I was finally able to overcome this limitation with GitHub actions using prettier --write, git diff, and diff2html. I created 2 GitHub actions: one to run on PRs and another to run on pushes. I used git diff -- . ':(exclude)package-lock.json' ':(exclude)package.json' because installing prettier currently changes these 2 files, so it gives a false positive in the diff.

The on push tests if the code is passing prettier --check. If it isn't it creates a git diff as an html file and upload it as an action artifact. The artifact currently is a zip file with the html diff inside, since viewing artifacts directly in browser is currently not supported.

name: Prettier code formatter (Push)

on:
  push:
    branches:
      - master
      - main
  workflow_dispatch:

jobs:
  check:
    # available images: https://github.com/actions/runner-images#available-images
    runs-on: ubuntu-latest
    steps:
      - name: Checkout 🛎️
        uses: actions/checkout@v4
      - name: Setup Node.js ⚙️
        uses: actions/setup-node@v4
      - name: Install Prettier 💾
        run: npm install --save-dev --save-exact prettier @shopify/prettier-plugin-liquid
      - name: Prettier Check 🔎
        id: prettier
        run: npx prettier . --check
      - name: Show diff 📝
        # https://docs.github.com/en/actions/learn-github-actions/expressions#failure
        if: ${{ failure() }}
        run: |
          npx prettier . --write
          git diff -- . ':(exclude)package-lock.json' ':(exclude)package.json' > diff.txt
          npm install -g diff2html-cli
          diff2html -i file -s side -F diff.html -- diff.txt
      - name: Upload html diff
        if: ${{ failure() && steps.prettier.conclusion == 'failure' }}
        uses: actions/upload-artifact@v4
        with:
          name: HTML Diff
          path: diff.html
          retention-days: 3

The on PR one adds a comment to the PR with the html content of the diff. This one I haven't tested yet since it involves this PR being accepted in the repo I commited it and then a next PR failing prettier test.

name: Prettier code formatter (PR)

on:
  pull_request:
    branches:
      - master
      - main
  workflow_dispatch:

jobs:
  check:
    # available images: https://github.com/actions/runner-images#available-images
    runs-on: ubuntu-latest
    steps:
      - name: Checkout 🛎️
        uses: actions/checkout@v4
      - name: Setup Node.js ⚙️
        uses: actions/setup-node@v4
      - name: Install Prettier 💾
        run: npm install --save-dev --save-exact prettier @shopify/prettier-plugin-liquid
      - name: Prettier Check 🔎
        id: prettier
        run: npx prettier . --check
      - name: Show diff 📝
        # https://docs.github.com/en/actions/learn-github-actions/expressions#failure
        if: ${{ failure() }}
        run: |
          npx prettier . --write
          git diff -- . ':(exclude)package-lock.json' ':(exclude)package.json' > diff.txt
          npm install -g diff2html-cli
          diff2html -i file -s side -F diff.html -- diff.txt
      - name: PR comment with html diff
        # https://docs.github.com/en/actions/learn-github-actions/expressions#failure-with-conditions
        if: ${{ failure() && steps.prettier.conclusion == 'failure' }}
        uses: thollander/actions-comment-pull-request@v2
        with:
          filePath: diff.html

@Osmose
Copy link

Osmose commented Jan 16, 2024

I'm still happy to rebase #12598 and fix any further feedback if someone can commit to reviewing and merging it. I simply don't have the access to do so.

@automaton82
Copy link

I'm a back-end dev coming mostly from C# and am fairly new to npm, node etc. I was surprised to see a linter not give information on what the issues are in a check, mostly because I cannot blindly allow it to modify files.

I have used eslint previously and definitely like the output from it very much. It's short, concise and tells you exactly what to do to fix it if you want to do it yourself.

Glad to see this ticket open (since 2019🫠) and yes a diff or really anything would help at this point.

@muescha
Copy link
Author

muescha commented Jan 22, 2024

also there exists an Feature Request for reporter:

@george-gca
Copy link

Yes, but it does not solve this issue, as pointed by @Osmose.

@thatsIch
Copy link

I found, that sometimes there are false positives or new situations where you do not want the code formatted like that. There is no way, that --write is 100% correct all the time. There is no feedback loop if you want to learn the rules.

#12598 would solve the conflict.

It does not modify the default "way" of prettier.

@joonashak
Copy link

Struggling with an issue where locally running prettier --check yields no errors and prettier -w yields no diff, yet in GitHub Actions, using Ubuntu 22, prettier --check fails on a single tsconfig.json.

I think this is definitely an example of a legit use case for getting a diff printout straight from Prettier. Yes, it can be achieved with a combination of prettier -w and git diff but that's assuming Prettier is used in a Git repo which might not always be the case.

@siawyoung
Copy link

siawyoung commented Feb 18, 2024

Struggling with an issue where locally running prettier --check yields no errors and prettier -w yields no diff, yet in GitHub Actions, using Ubuntu 22, prettier --check fails on a single tsconfig.json.

I think this is definitely an example of a legit use case for getting a diff printout straight from Prettier. Yes, it can be achieved with a combination of prettier -w and git diff but that's assuming Prettier is used in a Git repo which might not always be the case.

Oh man, this is happening to me now. It only happens for tsconfig.json files too! Not sure why.

EDIT: looks like it's been fixed in 3.2.5, thanks! @joonashak

https://github.com/prettier/prettier/blob/main/CHANGELOG.md#use-json-parser-for-tsconfigjson-by-default-16012-by-sosukesuzuki

@mesmere
Copy link

mesmere commented Mar 22, 2024

I don't even want this for CI usecases, I'm just trying out prettier and want to see what it'll do to my code before I clobber it all.

I could try to figure out the right incantations to get git to stash the changes I'm working on atm without stashing the prettier config itself, etc etc, so that git can revert --write's changes if I don't like them but it's a little silly that there's not just a dry run option in prettier itself. :x

@alexey-sh
Copy link

@mesmere have you considered to opt out prettier?

@webark
Copy link
Contributor

webark commented Mar 24, 2024

@alexey-sh BLASPHEMER!! 😂🙃

Another option would be to use the eslint plugin, which I think will show the errors when you run it through that?

https://github.com/prettier/eslint-plugin-prettier

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:cli Issues with Prettier's Command Line Interface status:needs discussion Issues needing discussion and a decision to be made before action can be taken
Projects
None yet
Development

No branches or pull requests