-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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
--pretty-er output by default #23408
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we... Have a way we can test this? Oh! Maybe We could write a unit test that shells out to the built local tsc and baselines the output!
Locals pretty straightforward; you got any details on what terms/apps this causes pretty by default in now?
8c5b163
to
7fd1dda
Compare
I know the last time I did this (i.e. when I originally wrote Tests are Monday-Daniel-and-Wesley's problem. 😄 |
src/compiler/tsc.ts
Outdated
function shouldBePretty(options: CompilerOptions) { | ||
if ((typeof options.pretty === "undefined" && typeof options.diagnosticStyle === "undefined") || options.diagnosticStyle === DiagnosticStyle.Auto) { | ||
return !!sys.writeOutputIsTty && sys.writeOutputIsTty(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spacing
src/compiler/commandLineParser.ts
Outdated
pretty: DiagnosticStyle.Pretty, | ||
simple: DiagnosticStyle.Simple, | ||
}), | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spacing
reportDiagnostic = createDiagnosticReporter(sys, /*pretty*/ true); | ||
} | ||
} | ||
|
||
function shouldBePretty(options: CompilerOptions) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So now there are two CLI flags that can tell tsc it's pretty. What if the user provides tsc --pretty false --diagnosticStyle pretty
or tsc --pretty true --diagnosticStyle simple
? Should there be a warning for providing both?
src/compiler/commandLineParser.ts
Outdated
@@ -56,6 +56,14 @@ namespace ts { | |||
category: Diagnostics.Command_line_Options, | |||
description: Diagnostics.Stylize_errors_and_messages_using_color_and_context_experimental | |||
}, | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather we do not add a new flag if we can. i think auto
is just pretty === undefined
.
if we really need a new value i would make pretty take boolean | "auto"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
boolean | "auto" | "simple" | "styled"
? (Where boolean toggles between auto
and styled
).
src/compiler/tsc.ts
Outdated
if ((typeof options.pretty === "undefined" && typeof options.diagnosticStyle === "undefined") || options.diagnosticStyle === DiagnosticStyle.Auto) { | ||
return !!sys.writeOutputIsTty && sys.writeOutputIsTty(); | ||
} | ||
return options.diagnosticStyle === DiagnosticStyle.Pretty || options.pretty; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!! options.pretty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please 1. remove the extra flag, and 2. test on chakra
src/compiler/sys.ts
Outdated
@@ -428,6 +428,7 @@ namespace ts { | |||
newLine: string; | |||
useCaseSensitiveFileNames: boolean; | |||
write(s: string): void; | |||
writeOutputIsTty?(): boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a reasonable name @weswigham?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TTY
, not Tty
.
src/compiler/tsc.ts
Outdated
reportDiagnostic = createDiagnosticReporter(sys, /*pretty*/ true); | ||
} | ||
} | ||
|
||
function shouldBePretty(options: CompilerOptions) { | ||
if ((typeof options.pretty === "undefined")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra parens.
29ff745
to
25bb581
Compare
Tried it out. tsc.exe (the Chakra host-wrapped |
I guess this is because tsc.exe doesn't set the console mode on Windows to enable processing of Virtual Terminal Sequences ( (I think Node.js parses these sequences itself and then calls the corresponding APIs like Can tsc.exe be updated to set this console mode? Thanks! |
Thanks for the pointers, I might look into it @kpreisser! |
This pull request always makes TypeScript's output
--pretty
when the compiler can detect whether its "output device" is appropriate for more colorful output.If TypeScript's output is a psuedo-TTY, then it will enable today's prettified output and emit escape characters to the console; however, when piping to another file or program, it will automatically turned off.
Programs that want to specify the behavior can specifically set
--pretty
, or the new--diagnosticStyle
flag.This PR also affords us the room to give more variation in out emit modes. For example, with
--diagnosticStyle
, we could have a more colorful simple/terse mode as requested in #10745.Fixes #10488.