Added an errors summary for --pretty --watch results by JoshuaKGoldberg · Pull Request #22254 · microsoft/TypeScript

JoshuaKGoldberg · 2018-03-01T04:34:45Z

Reports a "Found X error(s) across Y file(s)." message on each recompile if in both pretty and watch modes.

I ended up cutting & pasting emitFilesAndReportErrors up into three functions:

getProgramDiagnosticsAndEmit: Retrieves diagnostics output and emit results from the program.
reportDiagnosticErrors: Outputs the traditional error reporting that already existed.
summarizeDiagnosticsAcrossFiles: Outputs the new summary message.

This commit intentionally doesn't include tests yet as I have a few questions about implementation details:

In the first commit, there's an added message before the constant "Compilation complete." one. I would prefer to have one message with a descriptor of the errors or lack thereof, followed by "Waiting for file changes.". Would that break anything? Nothing comes to mind.
Why does emitFilesAndReportErrors exist in watch.ts but get used in tsc.ts? If it's part of logic not exclusive to logic, should it be moved to tsc.ts?
clearScreenIfNotWatchingForFileChanges is hardcoded to not clear the screen for one specific diagnostic method. I changed it to also allow the new messages. Is there a less hardcoded way I should use instead?

Commit 1 output in cmder:

Reports a "Found X error(s) across Y file(s)." message on each recompile if in both pretty and watch modes. This commit intentionally doesn't include tests as I have a few questions about implementation details. Will ask in PR first.

JoshuaKGoldberg · 2018-03-01T21:00:47Z

I'll take that signoff as approval :) and write tests & resolve conflicts soon.

sheetalkamat

I dont think this is great implementation.. I would rather modify existing emitFilesAndReportErrors to take additional parameter to summaryReporter which could either be undefined | reporter and if provided you write the summary using that reporter.

sheetalkamat · 2018-03-02T22:11:49Z

    }

+    function summarizeDiagnosticsAcrossFiles(diagnostics: Diagnostic[], reporter: WatchStatusReporter, newLine: string, compilerOptions: CompilerOptions): void {
+        if (diagnostics.length === 1) {


This is incorrect too.. This could be just compiler diagnostics or something.. Need not be in 1 file.

I'm tempted to say simplify and don't even bother with pluralization based on count. Other languages don't necessarily pluralize the same way.

could just be compiler diagnostics

How should it count the numbers of errors from diagnostics? diagnostic.category === DiagnosticCategory.Error, ignoring warnings & below?

simplify and don't even bother with pluralization

If a language doesn't pluralize for counts, then they would just have three localized messages with the same text. Is that bad? Seems fine to allow redundancy for nicer phrasing in most languages?

I think the multiple diagnostic messages are the correct approach here. the other one is to use error(s) instead, but i would rather we add the extra line and make it look pretty.

As for knowing which is which, you can filter the diagnostics on d.category === DiagnosticCategory.Error.

One thing we can simplify is removing the number of files altogether. since diagnostics can be global and not attached to any file. this will reduce the number of message also by one, and will simplify this function.

mhegazy · 2018-03-02T23:08:51Z

I wounder if we should show this all the time as well.. like not just --watch.. nu just regular compilations.

…hCompilerHost Instead of modifying the logic in both `tsc.ts`to `watch.ts` by splitting `emitFilesAndReportErrors` in two, this adds an optional function parameter to report them. Removes file counting for diagnostic messages, as they might not be coming from files. Next commit will include tests.

# Conflicts: # src/compiler/diagnosticMessages.json # src/compiler/watch.ts

JoshuaKGoldberg · 2018-03-09T17:42:09Z

Moved all the logic to watch.ts, so tsc.ts remains untouched.

@sheetalkamat I don't see any existing tests that include the "Compilation complete." string. Am I missing them, or do you want me to add them in this PR?

sheetalkamat · 2018-03-09T21:37:16Z

        emit(): EmitResult;
    }

+    /** @internal */


No need to have this internal since the namespace itself is internal

sheetalkamat · 2018-03-09T21:41:05Z

            }
        }

+        if (reportSummary) {


i agree with @mhegazy 's #22254 (comment) that we should just report this all the time

I'm hesitant to change the behavior for non-pretty non-watch users. What if those users have builds that rely on tsc not having any non-whitespace output? This would break their workflow.

If these changes are acceptable for watch mode, can we discuss enabling for them in a separate issue?

But that can be done through checking if (program.getCompilerOptions().pretty)

JoshuaKGoldberg · 2018-03-09T22:44:39Z

Ah, now the tests are failing. Will update...

sheetalkamat · 2018-03-09T22:49:29Z

+            const compilerOptions = builderProgram.getCompilerOptions();
+            const reportSummary = (errorCount: number) => {
+                if (errorCount === 1) {
+                    onWatchStatusChange(createCompilerDiagnostic(Diagnostics.Found_1_error, errorCount), sys.newLine, compilerOptions);


The new line character here used is incorrect, You dont want to use newline from sys but it should depend on compiler options and host.getNewLine()

mhegazy · 2018-03-29T22:47:23Z

@JoshuaKGoldberg #22570 should be fixed now, can we update this PR.

sheetalkamat · 2018-04-02T20:28:06Z

+            errors,
+            disableConsoleClears,
+            errors.length === 1
+                ? createCompilerDiagnostic(Diagnostics.Found_1_error)


Instead of doing this here, can you add the move this to https://github.com/Microsoft/TypeScript/pull/22254/files#diff-09ed66c9ac499e7fd991780ed486d800R96 and check if (postErrorsWatchDiagnostics.length) and do this there so that its in one place.

I don't think I can. If there's an exit that shouldn't display the "found X error(s)" message, namely checkOutputErrorsIncrementalWithExit, then the test should know not to expect the message:

1) tsc-watch program updates config file is deleted: AssertionError: ["13:14:33 - File change detected. Starting incremental compilation...\n\n\n","error TS6053: File '/a/b/tsconfig.json' not found.\n"]: expected 2 to equal 3 + expected - actual

...but +1 to DRY. Moved the creation into a small helper function; is that enough?

sheetalkamat · 2018-04-02T20:30:23Z

     * Helper that emit files, report diagnostics and lists emitted and/or source files depending on compiler options
     */
-    export function emitFilesAndReportErrors(program: ProgramToEmitFilesAndReportErrors, reportDiagnostic: DiagnosticReporter, writeFileName?: (s: string) => void) {
+    export function emitFilesAndReportErrors(program: ProgramToEmitFilesAndReportErrors, reportDiagnostic: DiagnosticReporter, reportSummary?: ReportEmitErrorSummary, writeFileName?: (s: string) => void) {


I think you want to change the definition to

export function emitFilesAndReportErrors(program: ProgramToEmitFilesAndReportErrors, reportDiagnostic: DiagnosticReporter, writeFileName: (s: string) => void, reportSummary?: ReportEmitErrorSummary)

currently tsc.ts's use of emitFilesAndReportErrors is broken ()

mhegazy · 2018-04-04T23:08:59Z

thanks @JoshuaKGoldberg !

Added an errors summary for --pretty --watch results

e0d067b

Reports a "Found X error(s) across Y file(s)." message on each recompile if in both pretty and watch modes. This commit intentionally doesn't include tests as I have a few questions about implementation details. Will ask in PR first.

mhegazy approved these changes Mar 1, 2018

View reviewed changes

sheetalkamat requested changes Mar 2, 2018

View reviewed changes

sheetalkamat reviewed Mar 2, 2018

View reviewed changes

Josh Goldberg added 3 commits March 8, 2018 20:47

Merge branch 'master' into pretty-watch-error-summaries

c5d2bb6

# Conflicts: # src/compiler/diagnosticMessages.json # src/compiler/watch.ts

Reverted now-unused ProgramDiagnosticsAndEmit

a4ca071

sheetalkamat reviewed Mar 9, 2018

View reviewed changes

Made watch mode always report summary

15b61dc

sheetalkamat reviewed Mar 9, 2018

View reviewed changes

JoshuaKGoldberg mentioned this pull request Mar 14, 2018

Unknown compiler options first printed before --pretty screen clears #22570

Closed

Merge branch 'master' into pretty-watch-error-summaries

d268279

JoshuaKGoldberg force-pushed the pretty-watch-error-summaries branch from 771a73d to d268279 Compare April 2, 2018 04:45

Fixed unit tests for error counts

cf0a0ec

sheetalkamat reviewed Apr 2, 2018

View reviewed changes

sheetalkamat requested changes Apr 2, 2018

View reviewed changes

Feedback: correct order in watch.ts; DRY tests

0dbebec

sheetalkamat approved these changes Apr 4, 2018

View reviewed changes

mhegazy merged commit e2bd282 into microsoft:master Apr 4, 2018

JoshuaKGoldberg deleted the pretty-watch-error-summaries branch April 4, 2018 23:11

JoshuaKGoldberg mentioned this pull request Apr 16, 2018

Consider combining the "Found X error(s)" and "Compilation" complete --pretty messages #23424

Closed

microsoft locked and limited conversation to collaborators Jul 25, 2018

Conversation

JoshuaKGoldberg commented Mar 1, 2018

Uh oh!

JoshuaKGoldberg commented Mar 1, 2018

Uh oh!

sheetalkamat 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!

mhegazy commented Mar 2, 2018

Uh oh!

JoshuaKGoldberg commented Mar 9, 2018

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!

JoshuaKGoldberg commented Mar 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhegazy commented Mar 29, 2018

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!

mhegazy commented Apr 4, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants