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

Prettified timestamps and error reports in --pretty #20416

Merged
merged 4 commits into from
Dec 19, 2017

Conversation

JoshuaKGoldberg
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg commented Dec 3, 2017

Timestamps look like Gulp's, with grey times inside white brackets.
Files have cyan filenames, yellow line and column numbers, and grey TS{####} errors. I wonder if those are actually useful for folks using the --pretty CLI: are they used for anything outside Visual Studio... Can we just get rid of them?

Re-uses compiler/program's color logic in compiler/watch. The relevant variables are now exported and marked @internal. Is there a preferred way of re-using this code in both those files?

Todo: fix test failures

Here are the after and before photos in Windows cmd...
image
image

...and cmder:
image
image

Timestamps look like Gulp's, with grey times inside white brackets.
Files have cyan filenames, yellow line and column numbers, and grey TS{####} errors. I wonder if those are actually useful for folks using the --pretty CLI: are they used for anything outside Visual Studio... Can we just get rid of them?

Re-uses compiler/program's color logic in compiler/watch. The relevant variables are now exported and marked `@internal`. Is there a preferred way of re-using this code in both those files?
@@ -241,22 +241,27 @@ namespace ts {
return errorMessage;
}

const redForegroundEscapeSequence = "\u001b[91m";
const yellowForegroundEscapeSequence = "\u001b[93m";
const blueForegroundEscapeSequence = "\u001b[93m";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: the blue sequence was the same as yellow before. I'm guessing that's a behavior bug that was never exposed. I've removed the sequence but left the behavior of using the yellow color in getCategoryFormat.

Copy link
Member

@weswigham weswigham Dec 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: the blue sequence was the same as yellow before. I'm guessing that's a behavior bug that was never exposed.

We don't actually use ErrorCategory.Warning right now (and Message is only used for help output and quickfixes/refactorings), so it would never get noticed/changed until it mattered, like for #19126

IMO, we should preemptively change Message to dark blue or something, just in case.

@weswigham
Copy link
Member

@DanielRosenwasser This seems reasonable and I like the slightly more colorful look; do you have any opinions?

@mhegazy
Copy link
Contributor

mhegazy commented Dec 4, 2017

Just me pontificating here.. would be better to have the error message slightly different shade from the code below it, just to create a separation between the two sections?

@JoshuaKGoldberg
Copy link
Contributor Author

slightly different shade

That would be great, but AFAIK we're limited to very basic console colors (white, grey, yellow, etc.) using the way it resets them now. Let's look into more advanced things & how chalk&co do them after this?

@weswigham
Copy link
Member

I believe 16million color escapes read like \u001B[38;2;r;g;bm where r, g, and b are the weights of each of those colors. I think 256 color escapes are \u001B[38;5;cm where c is some color code. Not sure, though. Also would probably need feature detection of some kind.

const yellowForegroundEscapeSequence = "\u001b[93m";
const blueForegroundEscapeSequence = "\u001b[93m";
/** @internal */
export const foregroundColorEscapeSequences = {
Copy link
Member

@DanielRosenwasser DanielRosenwasser Dec 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can just be a const string enum ForegroundColorEscapeSequence, and it doesn't need to be exported at all

@@ -48,6 +48,14 @@ namespace ts {
};
}

export function createWatchDiagnosticReporterWithColor(system = sys): DiagnosticReporter {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I just saw this. It's unfortunate that we need a new reporter. This should probably be marked /** @internal */ though.

@DanielRosenwasser DanielRosenwasser merged commit ccd5608 into microsoft:master Dec 19, 2017
@DanielRosenwasser
Copy link
Member

Thanks @JoshuaKGoldberg!

@JoshuaKGoldberg
Copy link
Contributor Author

Whoo!

@mihailik
Copy link
Contributor

mihailik commented Dec 19, 2017

Style suggestions:

  • the output starts to look like Christmas tree -- few too many bright lights
  • highlight to catch people's eye on relevant detail, fade out mundane
  • best to limit number of colours, use variation of dark/light

ASCII escapes generally support 2 shades per colour (three for grey/silver/white). Same applies to pure Windows console colours, of course.

screen shot 2017-12-19 at 21 31 40

What's worth noticing in timed banners is seconds - those are most likely to differ, not whole timestamps, nor brackets around.

Filename, position, error labels all relate to an error -- ideally all share the same general colour (red). Also worth highlighting line number, but fading out character offset.

Error message is the key piece of info, having it as bright white make it stand out and very readable.

@microsoft microsoft locked and limited conversation to collaborators Jun 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants