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 summary of errors to compiler output #28196

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@ashfurrow

ashfurrow commented Oct 28, 2018

Hello! I'm a big fan of TypeScript and I found an issue, #14211, that I thought I could help out with. Other compilers I've worked with, in Swift/ObjC and Scala, usually output a compilation summary to go along with the errors; this PR adds that summary to the TypeScript compiler output:

screen shot 2018-10-28 at 7 25 36 pm

I looked into adding duration of the compiler run to the output, as suggested in the issue, but it doesn't look like the duration is collected without diagnostics enabled.

Some concerns were brought up in that issue around changing the compiler output, but it was confirmed that popular tools that read the logs do so using Regex, so adding something like this shouldn't break anything. I should note that this will print the summaries whether or not in pretty mode, which I think is pretty standard among compilers. Happy to discuss, though.

Beyond the baseline snapshots, I looked for unit tests specific to the code I changed but couldn't find any. Let me know if I missed something.

Very open to feedback – thanks again for all your hard work on TypeScript!

Fixes #14211.

@@ -226,6 +226,11 @@ namespace ts {
for (const diagnostic of diagnostics) {
output += formatDiagnostic(diagnostic, host);
}
output += host.getNewLine();

This comment has been minimized.

@sheetalkamat

sheetalkamat Oct 29, 2018

Member

I don't think this is correct change. We already report summary in tsc watch mode and that's in emitFilesAndReportErrors

This comment has been minimized.

@ashfurrow

ashfurrow Oct 29, 2018

Right, that makes sense. We should copy that functionality from watch mode then? I really think that the TypeScript compiler should output this information in both modes, by default. But let me know.

@sheetalkamat

I don't know if we want this all the time. (may be just in pretty mode?) @DanielRosenwasser to comment on that.

Show resolved Hide resolved src/compiler/program.ts Outdated
@ashfurrow

This comment has been minimized.

ashfurrow commented Oct 30, 2018

I'm hoping this PR doesn't get stalled, and so I don't mean to be annoying but I'm curious what it would take to get this approved? I'm happy to discuss implementations, etc, just let me know!

I know this feature might seem controversial at first glance, but it would make a big difference for day-to-day developer experiences and as discussed in #14211, it's an additive change that shouldn't impact other existing projects.

@sheetalkamat

This comment has been minimized.

Member

sheetalkamat commented Oct 30, 2018

Ping @DanielRosenwasser to suggest expected behavior (do we report summary like watch) only in pretty mode or irrespectively?

@DanielRosenwasser

This comment has been minimized.

Member

DanielRosenwasser commented Oct 31, 2018

I like the idea of providing this in --pretty mode (which is now the default anyway). I think @Tyriar could probably weigh in on whether this could cause any problems with problem matching.

Show resolved Hide resolved src/compiler/program.ts Outdated
@Tyriar

This comment has been minimized.

Member

Tyriar commented Oct 31, 2018

@dbaeumer @alexr00 for the question in #28196 (comment) about impact on problem matchers/tasks.

@ashfurrow ashfurrow force-pushed the ashfurrow:summary-of-issues branch from 5477943 to 028b867 Oct 31, 2018

@ashfurrow ashfurrow force-pushed the ashfurrow:summary-of-issues branch from 028b867 to daf4df4 Nov 1, 2018

@sheetalkamat

This comment has been minimized.

Member

sheetalkamat commented Nov 1, 2018

Closing it in favor of #28300

@ashfurrow ashfurrow deleted the ashfurrow:summary-of-issues branch Nov 2, 2018

@ashfurrow

This comment has been minimized.

ashfurrow commented Nov 2, 2018

@sheetalkamat Okay, thanks for continuing this! I'm excited that this is getting added to TypeScript, and I learned a lot going through the code review process. Thank you (and others) for the feedback 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment