Don't use ANSI codes if stdout isn't a TTY by Narigo · Pull Request #2903 · prettier/prettier

Narigo · 2017-09-27T00:33:40Z

I've tried to pipe the output of --list-differences into xargs git add, but due to the progress information, the stdout is not really usable.

This commit could fix that by suppressing output information. I think it might make sense to change process.stdout/console.log to process.stderr/console.error in case of errors, but I did not want to put multiple things into one PR.

Doing yarn did not work for me as it currently cannot find the Git commit of postcss-values-parser referenced in the package.json. I've installed the latest version of it to get it to run, but this is probably the cause why the css_numbers/jsfmt.spec.js test fails for me locally.

Edit: Forgot yarn lint initially, which is now fixed :)

lydell · 2017-09-27T05:41:40Z

About the yarn install error – sorry, that was by bad. It should work now again. See #2627 (comment)

lydell · 2017-09-28T18:00:11Z

What if we printed the progress information to stderr instead?

suchipi · 2017-09-28T18:02:05Z

I'm concerned that printing to stderr might flag the run as an error in some CI servers...

Also, I personally would prefer the name --no-progress over --silent

lydell · 2017-09-28T18:22:13Z

Warnings are usually printed to stderr, and I have not seen CI failures because of warnings. As far as I know, only the exit code matters.

Narigo · 2017-09-28T21:06:03Z

--no-progress is better, yes. Could also be --quiet. Just tell me what you prefer, if you think adding this feature makes sense at all and I will change it.

azz · 2017-09-29T03:06:06Z

Can we use process.stdout.isTTY instead of a new option?

Narigo · 2017-09-29T07:30:25Z

Will change it to process.stdout.isTTY and remove the option.

Signed-off-by: Joern Bernhardt <[email protected]>

Narigo · 2017-09-29T18:32:20Z

Should be updated now and use process.stdout.isTTY.

lydell

Looks good! I made a few small comments, though.

lydell · 2017-09-29T18:56:20Z

+
+const runPrettier = require("../runPrettier");
+
+test("output with --list-different + unformated differs when piped", () => {


Typo: "formated" → "formatted"

There are more instances of this below, including some file names.

I've copied the texts from the write.js tests. Shall I rename the files in there as well or do you want to just have the string typos fixed and the file changes in another PR?

I see. Let's do it in another PR (if you feel like it).

lydell · 2017-09-29T18:57:51Z

+exports[`no file diffs with --list-different + formated file 2`] = `Array []`;
+
+exports[`output with --list-different + unformated differs when piped 1`] = `
+"unformated.js[2K[1Gunformated.js


Can you add --no-color to these tests? We've had CI failures because of this before. (Also, the tests become more readable.)

Signed-off-by: Joern Bernhardt <[email protected]>

lydell

LGTM

lydell · 2017-09-29T19:24:55Z

+exports[`no file diffs with --list-different + formatted file 2`] = `Array []`;
+
+exports[`output with --list-different + unformatted differs when piped 1`] = `
+"unformated.js[2K[1Gunformated.js


I thought this was a color code. Oh well, --no-color doesn't hurt anyway. I guess it's from moving the cursor.

Yes, and that's exactly the problem I had when piping the output: The next command in the pipe complained about that... :)

Narigo · 2017-09-29T19:42:30Z

I'd rename formated.js -> formatted.js and unformated.js -> unformatted.js, but I'd like to wait until this is merged. Otherwise I'd have to change the other things first, rebase here and ... I admit I just finished the renaming on this branch and am now too lazy to do it the other way around... ;)

suchipi · 2017-09-29T22:19:52Z


  process.chdir(normalizeDir(dir));
  process.stdin.isTTY = !!options.isTTY;
+  process.stdout.isTTY = !!options.stdoutIsTTY;


I think we can just use the existing isTTY here; I don't know of any real-world situations where stdin is a tty and stdout isn't (or vice-versa)

https://nodejs.org/api/process.html

$ node -p "Boolean(process.stdin.isTTY)" true $ echo "foo" | node -p "Boolean(process.stdin.isTTY)" false $ node -p "Boolean(process.stdout.isTTY)" true $ node -p "Boolean(process.stdout.isTTY)" | cat false

I think it is correct to use process.stdout.isTTY here.

mitermayer · 2017-09-30T01:17:32Z

Would this issue also solve something like this prettier/vim-prettier#59?

Having issues now with prettier 1.7.1 since we are printting the message

--parser with value postcss is deprecated. Prettier now treats it as: --parser=css.

Sad :(

azz · 2017-09-30T02:22:17Z

Would this issue also solve something like this prettier/vim-prettier#59?

I don't think so. That warning should be printed with console.warn which should go to stderr. @ikatyang any ideas?

ikatyang · 2017-09-30T05:37:51Z

@mitermayer

If vim-prettier somehow can't understand the difference between stdout/stderr, I'd recommend to write a wrapper script to execute prettier so that you can do whatever you want, e.g. hide warnings.

const child_process = require("child_process");
const pathToPrettierCli = "node_modules/.bin/prettier";

const result = child_process.spawnSync(pathToPrettierCli, ["--parser", "postcss"], {
  encoding: "utf8",
  input: ".test { display: none; }"
});

console.log('stdout', result.stdout);
console.log('stderr', result.stderr);
console.log('status', result.status);

@azz

Should we add some kind of logger system so that they can easily get what they want and we can also log some debug information so as to trace what the root cause is? e.g. --loglevel.

azz · 2017-09-30T05:46:50Z

Sounds good to me.

azz · 2017-10-05T11:02:48Z

Renamed the PR and merged. Thanks!

azz · 2017-10-05T12:31:16Z

This broke this build.

https://travis-ci.org/prettier/prettier/jobs/283636656

Summary of all failing tests
FAIL tests_integration/__tests__/piped-output.js
  ● output with --list-different + unformatted differs when piped
    expect(value).toMatchSnapshot()
    
    Received value does not match stored snapshot 1.
    
    - "unformated.jsated.js
    "
    + undefined
      
      at Object.<anonymous>.test (tests_integration/__tests__/piped-output.js:18:26)
          at Promise (<anonymous>)
          at <anonymous>
  ● no file diffs with --list-different + formatted file
    expect(value).toMatchSnapshot()
    
    Received value does not match stored snapshot 1.
    
    - Array []
    + undefined
      
      at Object.<anonymous>.test (tests_integration/__tests__/piped-output.js:41:25)
          at Promise (<anonymous>)
          at <anonymous>
FAIL tests_integration/__tests__/write.js
  ● write file with --write + unformated file › (stdout)
    expect(value).toMatchSnapshot()
    
    Received value does not match stored snapshot 1.
    
    - Snapshot
    + Received
    
    - "unformated.js%s %dms
    + "%s %dms
      "
      
      at Object.test (tests_integration/runPrettier.js:106:25)
          at Promise (<anonymous>)
          at <anonymous>
  ● do not write file with --write + formated file › (stdout)
    expect(value).toMatchSnapshot()
    
    Received value does not match stored snapshot 1.
    
    - Snapshot
    + Received
    
    - "formated.js%s %dms
    + "%s %dms
      "
      
      at Object.test (tests_integration/runPrettier.js:106:25)
          at Promise (<anonymous>)
          at <anonymous>
  ● do not write file with --write + invalid file › (stdout)
    expect(value).toMatchSnapshot()
    
    Received value does not match stored snapshot 1.
    
    - Snapshot
    + Received
    
    - "invalid.js
      "
    + "
      
      at Object.test (tests_integration/runPrettier.js:106:25)
          at Promise (<anonymous>)
          at <anonymous>

Narigo · 2017-10-05T12:46:17Z

I have no idea. After fetching again, I see that my branch diverged from yours. Probably some git issue?
Did the behaviour on --write change in the meantime? I've copied most of the tests from the write integration tests.

azz · 2017-10-05T12:56:31Z

On mobile at the moment but maybe #2957 or #2951

Narigo force-pushed the add-silent-option branch from b372624 to 1f3ca64 Compare September 29, 2017 18:22

Use process.stdout.isTTY to suppress progress output if used in a pipe

4d79b74

Signed-off-by: Joern Bernhardt <[email protected]>

Narigo force-pushed the add-silent-option branch from 1f3ca64 to 4d79b74 Compare September 29, 2017 18:30

lydell requested changes Sep 29, 2017

View reviewed changes

Add --no-color option and fix typo in test description

58834d0

Signed-off-by: Joern Bernhardt <[email protected]>

lydell approved these changes Sep 29, 2017

View reviewed changes

suchipi reviewed Sep 29, 2017

View reviewed changes

ikatyang mentioned this pull request Sep 30, 2017

Add --loglevel option #2939

Closed

azz changed the title ~~Add --silent option~~ Don't use ANSI codes if stdout isn't a TTY Oct 5, 2017

azz merged commit 2011053 into prettier:master Oct 5, 2017

Narigo deleted the add-silent-option branch October 5, 2017 11:03

azz mentioned this pull request Oct 5, 2017

Include duration in --write snapshots #2960

Merged

Narigo mentioned this pull request Oct 6, 2017

Fix typo in test files (*formated -> *formatted) #2982

Merged

lock Bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jan 19, 2019

lock Bot locked as resolved and limited conversation to collaborators Jan 19, 2019


		const runPrettier = require("../runPrettier");

		test("output with --list-different + unformated differs when piped", () => {

Uh oh!

Conversation

Narigo commented Sep 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lydell commented Sep 27, 2017

Uh oh!

lydell commented Sep 28, 2017

Uh oh!

suchipi commented Sep 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lydell commented Sep 28, 2017

Uh oh!

Narigo commented Sep 28, 2017

Uh oh!

azz commented Sep 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Narigo commented Sep 29, 2017

Uh oh!

Narigo commented Sep 29, 2017

Uh oh!

lydell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lydell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Narigo commented Sep 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mitermayer commented Sep 30, 2017

Uh oh!

azz commented Sep 30, 2017

Uh oh!

ikatyang commented Sep 30, 2017

Uh oh!

azz commented Sep 30, 2017

Uh oh!

azz commented Oct 5, 2017

Uh oh!

azz commented Oct 5, 2017

Uh oh!

Narigo commented Oct 5, 2017

Uh oh!

azz commented Oct 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Narigo commented Sep 27, 2017 •

edited

Loading

suchipi commented Sep 28, 2017 •

edited

Loading

azz commented Sep 29, 2017 •

edited

Loading