GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — --pretty-er output by default by DanielRosenwasser · Pull Request #23408 · microsoft/TypeScript

Conversation

@DanielRosenwasser

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) commented Apr 14, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
JoshuaKGoldberg reacted with heart emoji
All reactions
  • ❤️ 1 reaction
@DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) requested review from Mohamed Hegazy (mhegazy) and Wesley Wigham (weswigham) April 14, 2018 04:08
@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) changed the title Automatically enable --pretty Enable --pretty automatically Apr 14, 2018
@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) changed the title Enable --pretty automatically --pretty-er output by default Apr 14, 2018
weswigham
Wesley Wigham (weswigham) approved these changes Apr 14, 2018
View reviewed changes

@weswigham Wesley Wigham (weswigham) left a comment

Copy link
Copy Markdown
Member

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Daniel Rosenwasser (DanielRosenwasser) added 2 commits April 13, 2018 21:19
@DanielRosenwasser
Automatically configure tsc output and provide a new 'diagnosticStyle…
bd3e854 
…' option.
@DanielRosenwasser
Accepted baselines
7fd1dda
@DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) force-pushed the autoPretty branch from 8c5b163 to 7fd1dda Compare April 14, 2018 04:19
@DanielRosenwasser

Daniel Rosenwasser (DanielRosenwasser) commented Apr 14, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

I know the last time I did this (i.e. when I originally wrote --pretty), there were issues with either Cygwin's bash terminal or MinGW. I forgot which, but I'll have to peek at that again.

Tests are Monday-Daniel-and-Wesley's problem. 😄
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
alfaproject
João Dias (alfaproject) reviewed Apr 14, 2018
View reviewed changes
Comment thread 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();
}

@alfaproject João Dias (alfaproject) Apr 14, 2018

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
alfaproject
João Dias (alfaproject) reviewed Apr 14, 2018
View reviewed changes
Comment thread src/compiler/commandLineParser.ts Outdated
pretty: DiagnosticStyle.Pretty,
simple: DiagnosticStyle.Simple,
}),
},

@alfaproject João Dias (alfaproject) Apr 14, 2018

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
DanielRosenwasser reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@DanielRosenwasser
Fix spacing.
299002d
JoshuaKGoldberg
Josh Ghoulberg 👻 (JoshuaKGoldberg) reviewed Apr 16, 2018
View reviewed changes
Comment thread src/compiler/tsc.ts
}
}

function shouldBePretty(options: CompilerOptions) {

@JoshuaKGoldberg Josh Ghoulberg 👻 (JoshuaKGoldberg) Apr 16, 2018

Copy link
Copy Markdown
Contributor

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
mhegazy
Mohamed Hegazy (mhegazy) reviewed Apr 17, 2018
View reviewed changes
Comment thread src/compiler/commandLineParser.ts Outdated
category: Diagnostics.Command_line_Options,
description: Diagnostics.Stylize_errors_and_messages_using_color_and_context_experimental
},
{

@mhegazy Mohamed Hegazy (mhegazy) Apr 17, 2018

Copy link
Copy Markdown
Contributor

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"

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@weswigham Wesley Wigham (weswigham) Apr 17, 2018

Copy link
Copy Markdown
Member

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).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
mhegazy
Mohamed Hegazy (mhegazy) reviewed Apr 17, 2018
View reviewed changes
Comment thread 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;

@mhegazy Mohamed Hegazy (mhegazy) Apr 17, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
mhegazy
Mohamed Hegazy (mhegazy) approved these changes Apr 17, 2018
View reviewed changes

@mhegazy Mohamed Hegazy (mhegazy) left a comment

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) commented Apr 18, 2018
View reviewed changes
Comment thread src/compiler/sys.ts Outdated
newLine: string;
useCaseSensitiveFileNames: boolean;
write(s: string): void;
writeOutputIsTty?(): boolean;

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) Apr 18, 2018

Copy link
Copy Markdown
Member Author

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 Wesley Wigham (@weswigham)?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@weswigham Wesley Wigham (weswigham) Apr 18, 2018

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@DanielRosenwasser
flags--;
6953fa1
mhegazy
Mohamed Hegazy (mhegazy) reviewed Apr 18, 2018
View reviewed changes
Comment thread src/compiler/tsc.ts Outdated
}

function shouldBePretty(options: CompilerOptions) {
if ((typeof options.pretty === "undefined")) {

@mhegazy Mohamed Hegazy (mhegazy) Apr 18, 2018

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Daniel Rosenwasser (DanielRosenwasser) added 2 commits April 18, 2018 12:33
@DanielRosenwasser
Address CR feedback.
320cb40
@DanielRosenwasser
Accepted baselines.
25bb581
@DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) force-pushed the autoPretty branch from 29ff745 to 25bb581 Compare April 18, 2018 19:43
@DanielRosenwasser

Daniel Rosenwasser (DanielRosenwasser) commented Apr 19, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Tried it out. tsc.exe (the Chakra host-wrapped tsc) reverts to the old behavior. Like before, --pretty on tsc.exe produces garbled text when explicitly specified, but that's always been the behavior. I'm going to pull this in so we can get it in tonight's nightly release.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) merged commit 6ee4989 into master Apr 19, 2018
@DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) deleted the autoPretty branch April 19, 2018 06:10
@mhegazy

Mohamed Hegazy (mhegazy) commented Apr 19, 2018

Copy link
Copy Markdown
Contributor

Daniel Rosenwasser (@DanielRosenwasser) please update https://github.com/Microsoft/TypeScript-Handbook/blob/master/pages/Compiler%20Options.md as well.
DanielRosenwasser reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kpreisser

Konstantin Preißer (kpreisser) commented Apr 23, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Hi Daniel Rosenwasser (@DanielRosenwasser),

Like before, --pretty on tsc.exe produces garbled text when explicitly specified, but that's always been the behavior.

I guess this is because tsc.exe doesn't set the console mode on Windows to enable processing of Virtual Terminal Sequences (ENABLE_VIRTUAL_TERMINAL_PROCESSING).
Starting with Windows 10 Version 1511, this mode can be set to enable the parsing of terminal sequences, so that --pretty should work with tsc.exe on Windows 10.

(I think Node.js parses these sequences itself and then calls the corresponding APIs like SetConsoleTextAttribute, so that it works also with older Windows versions.)

Can tsc.exe be updated to set this console mode?

Thanks!
DanielRosenwasser and Jessidhia reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@DanielRosenwasser

Daniel Rosenwasser (DanielRosenwasser) commented Apr 24, 2018

Copy link
Copy Markdown
Member Author

Thanks for the pointers, I might look into it Konstantin Preißer (@kpreisser)!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) mentioned this pull request Apr 28, 2018
--pretty by default in tsc.exe (the ChakraCore tsc host) #23754
Closed
@microsoft Microsoft (microsoft) locked and limited conversation to collaborators Jul 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@weswigham Wesley Wigham (weswigham) weswigham approved these changes
+3 more reviewers
@alfaproject João Dias (alfaproject) alfaproject left review comments
@JoshuaKGoldberg Josh Ghoulberg 👻 (JoshuaKGoldberg) JoshuaKGoldberg left review comments
@mhegazy Mohamed Hegazy (mhegazy) mhegazy approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@DanielRosenwasser @mhegazy @kpreisser @alfaproject @weswigham @JoshuaKGoldberg
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.