fix(45713) Improve error report summaries #45742

rbargholz · 2021-09-06T11:11:00Z

This PR intends to improve the error report summaries that get generated from diagnostics. Cases where there are multiple errors: "4 errors in 2 files" as well as single errors, "1 error in testA.ts:4" are accounted for as stated in the issue.

Fixes #45713

ghost · 2021-09-06T11:11:12Z

All CLA requirements met.

rbargholz · 2021-09-13T02:29:31Z

@orta Code updated:

Added tabular data output of files and their errors when there is more than one error.

orta · 2021-09-13T13:45:37Z

This is looking great! It looks feature complete to me?

@typescript-bot pack this

typescript-bot · 2021-09-13T13:45:40Z

Heya @orta, I've started to run the tarball bundle task on this PR at 08906dd. You can monitor the build here.

rbargholz · 2021-09-13T13:50:54Z

This is looking great! It looks feature complete to me?

@orta Thank you! I am just thoughtful, imagine you had a file with 12 errors, I don't know how common it is, but it could push the table out by one space. Do you think it's an edge case worth handling, or will it likely not occur?

typescript-bot · 2021-09-13T14:03:44Z

Hey @orta, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/110389/artifacts?artifactName=tgz&fileId=B711E9590F88354A7E3B9EEFEB52F791FD1A7586247CB188AFB71F0A3B463FEB02&fileName=/typescript-4.5.0-insiders.20210913.tgz"
    }
}

and then running npm install.

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/[email protected]".;

orta · 2021-09-13T20:27:14Z

No worries: for that you would push it in!

Errors   Files
     1   /src/project/src/indirectUse.ts:2
    12   /src/project/src/directUse.ts:2
   123   /src/project/src/indirectUse.ts:2

It can go pretty high:

Errors   Files
     1   /src/project/src/indirectUse.ts:2
    12   /src/project/src/directUse.ts:2
123456   /src/project/src/indirectUse.ts:2

But what if you have over 999,999 (a million!) errors in a single file? I think at that point it is ok to break the table

Errors   Files
     1   /src/project/src/indirectUse.ts:2
    12   /src/project/src/directUse.ts:2
1000000   /src/project/src/indirectUse.ts:2

Mainly because I don't really think that will ever happen 🥇 - kudos if this hits someone though

rbargholz · 2021-09-14T05:49:47Z

@orta Code updated

Fixed lint issues
Error numbers will now push in til it caps out at 6 digits, if someone has written code that has literally a MILLION errors, the table will be pushed out by one space.

rbargholz · 2021-09-14T05:50:05Z

Untagged WIP

orta

Great work, only a few ideas for discussion - don't worry about the rebase, I can handle all that post reviews 👍🏻

orta · 2021-09-14T16:19:22Z

src/compiler/watch.ts

+        tabularData += headerRow;
+        distinctFiles.forEach(file => {
+            const errorCountForFile = countWhere(filesInError, fileInError => fileInError!.fileName === file!.fileName);
+            const errorCountDigitsLength = Math.log(errorCountForFile) * Math.LOG10E + 1 | 0;


a less abstract alternative could be String(errorCountForFile).length (unless I'm missing something)

@orta I agree that could also work. I'm happy to update it, it's probably easier to read. I just felt it was a little strange to convert to a string to get the length and nothing else.

orta · 2021-09-14T16:22:01Z

src/compiler/watch.ts

+                        Diagnostics.Found_0_errors_in_1_file :
+                        Diagnostics.Found_0_errors_in_1_files,
+                errorCount,
+                distinctFileNamesWithLines.length);


This is quite a dense statement, not sure I have any good ideas off the top off my head on how to improve it though

I'll have a look back at it and see what I could do. I also try to avoid double ternaries if I can avoid it.

What about something like this?

if(distinctFileNamesWithLines.length === 0 && errorCount !== 1) { const foundErrorsWithNoFilesDiagnostic = createCompilerDiagnostic( Diagnostics.Found_0_errors, errorCount); return `${newLine}${flattenDiagnosticMessageText(foundErrorsWithNoFilesDiagnostic.messageText, newLine)}${newLine}${newLine}`; } const d = errorCount === 1 ? createCompilerDiagnostic( filesInError[0] !== undefined ? Diagnostics.Found_1_error_in_1 : Diagnostics.Found_1_error, errorCount, distinctFileNamesWithLines[0]) : createCompilerDiagnostic( distinctFileNamesWithLines.length === 1 ? Diagnostics.Found_0_errors_in_1_file : Diagnostics.Found_0_errors_in_1_files, errorCount, distinctFileNamesWithLines.length); return `${newLine}${flattenDiagnosticMessageText(d.messageText, newLine)}${newLine}${newLine}${errorCount > 1 ? createTabularErrorsDisplay(nonNilFiles) : ""}${newLine}`;

Works for me, wes also said this is pretty common in the codebase, so the original is also fine 👍🏻

orta · 2021-09-14T16:24:25Z

tests/baselines/reference/tsc/incremental/initial-build/noEmit-changes-composite.js


+Errors   Files
+     1   /src/project/src/directUse.ts:2
+     1   /src/project/src/indirectUse.ts:2
 exitCode:: ExitStatus.DiagnosticsPresent_OutputsGenerated



In comparison to the original I think we might need an extra newline after the table to be consistent with our newlines 👍🏻

Got it, I'll add an extra newline.

weswigham · 2021-09-14T17:58:49Z

src/compiler/watch.ts

+        const distinctFiles = filesInError.filter((value, index, self) => index === self.findIndex(file => file!.fileName === value!.fileName));
+        if(distinctFiles.length === 0) return tabularData;
+
+        const headerRow = "Errors   Files\n";


This probably needs to be a localized message~

If we do that, (which I think we probably do need to do) then the numbers about length etc will need to not be hardcoded anymore. For example the Chinese version of "errors" might be 1-2 characters characters wide.

I didn't think of that, yes this should probably be localised.

@orta

If we do that, (which I think we probably do need to do) then the numbers about length etc will need to not be hardcoded anymore

May I ask how you mean by "hardcoded" here? Do you mean we don't need to bother so much about left padding?

By hardcoding, I mean const BASE_LEFT_PADDING = 6; only makes sense when 'errors' the string is 6 characters long for example.

Armed with google translate, perhaps this is a reasonable design answer:

错误案卷 3 build/lib/builtInExtensionsCG.ts

错误案卷 12 build/lib/builtInExtensionsCG.ts 123 extensions/html-language-features/client/src/htmlClient.ts

错误案卷 12 build/lib/builtInExtensionsCG.ts 123 extensions/html-language-features/client/src/htmlClient.ts 1234 extensions/html-language-features/client/src/htmlEmptyTagsShared.ts

Where tsc pulls out the longest length of a value from either the highest number of errors in a file or the translated word for "errors", allowing people to have a MAX_INT amount of errors because it just moves the column to the right once it hits a particular size

错误案卷 12 build/lib/builtInExtensionsCG.ts 123 extensions/html-language-features/client/src/htmlClient.ts 1234 extensions/html-language-features/client/src/htmlEmptyTagsShared.ts 123456789 extensions/html-language-features/client/src/tagClosing.ts

Errors Files 12 build/lib/builtInExtensionsCG.ts 123 extensions/html-language-features/client/src/htmlClient.ts 1234 extensions/html-language-features/client/src/htmlEmptyTagsShared.ts 123456789 extensions/html-language-features/client/src/tagClosing.ts

Does that feel reasonable to you?

That will work for me too. I'll work on it and see what I can come up with. Thank you for the explanation.

…e to localization

orta · 2021-10-01T14:36:34Z

OK, now it can handle when the number of errors is greater than the length of the title too:

I also left a note for localizers to try and make sure that message doesn't get mangled in localization in the JSON.

rbargholz · 2021-10-02T01:30:53Z

@orta Apologies for not getting back onto this, life got in the way.

…ader due to localization - Fixed baseline error - Fixed linter error

orta · 2021-10-02T11:47:55Z

Never feel guilty for not contributing to OSS in your spare time. I'm here to also help getting it in 👍🏻

orta · 2021-12-07T10:50:34Z

OK, I think this is good to go 👍🏻

orta · 2021-12-07T10:50:56Z

Thanks @rbargholz!

* Improve error report summaries (microsoft#45713) * fixup! Improve error report summaries (microsoft#45713) * fixup! fixup! Improve error report summaries (microsoft#45713) * Adds support for handling localization renaming the 'files' header due to localization * fixup! Adds support for handling localization renaming the 'files' header due to localization - Fixed baseline error - Fixed linter error Co-authored-by: Orta <[email protected]> Co-authored-by: Orta Therox <[email protected]>

Improve error report summaries (microsoft#45713)

0638417

typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Sep 6, 2021

rbargholz changed the title ~~[WIP] Improve error report summaries (#45713)~~ fix (45713)[WIP] Improve error report summaries Sep 6, 2021

rbargholz changed the title ~~fix (45713)[WIP] Improve error report summaries~~ fix(45713) [WIP] Improve error report summaries Sep 6, 2021

orta self-assigned this Sep 6, 2021

fixup! Improve error report summaries (microsoft#45713)

08906dd

fixup! fixup! Improve error report summaries (microsoft#45713)

1af84d2

rbargholz changed the title ~~fix(45713) [WIP] Improve error report summaries~~ fix(45713) Improve error report summaries Sep 14, 2021

orta requested review from sandersn and weswigham September 14, 2021 16:16

orta reviewed Sep 14, 2021

View reviewed changes

weswigham requested changes Sep 14, 2021

View reviewed changes

Adds support for handling localization renaming the 'files' header du…

f543c40

…e to localization

typescript-bot added For Backlog Bug PRs that fix a backlog bug and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Oct 1, 2021

Merge branch 'main' into improved_error_summary

b2988e7

fixup! Adds support for handling localization renaming the 'files' he…

db56f9f

…ader due to localization - Fixed baseline error - Fixed linter error

orta merged commit 7a12909 into microsoft:main Dec 7, 2021

fix(45713) Improve error report summaries #45742

fix(45713) Improve error report summaries #45742

Uh oh!

Conversation

rbargholz commented Sep 6, 2021 • edited by orta Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Sep 6, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rbargholz commented Sep 13, 2021

Uh oh!

orta commented Sep 13, 2021

Uh oh!

typescript-bot commented Sep 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rbargholz commented Sep 13, 2021

Uh oh!

typescript-bot commented Sep 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

orta commented Sep 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rbargholz commented Sep 14, 2021

Uh oh!

rbargholz commented Sep 14, 2021

Uh oh!

orta left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rbargholz Sep 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

orta commented Oct 1, 2021

Uh oh!

rbargholz commented Oct 2, 2021

Uh oh!

orta commented Oct 2, 2021

Uh oh!

orta commented Dec 7, 2021

Uh oh!

orta commented Dec 7, 2021

Uh oh!

Uh oh!

rbargholz commented Sep 6, 2021 •

edited by orta

Loading

ghost commented Sep 6, 2021 •

edited by ghost

Loading

typescript-bot commented Sep 13, 2021 •

edited

Loading

typescript-bot commented Sep 13, 2021 •

edited

Loading

orta commented Sep 13, 2021 •

edited

Loading

orta left a comment •

edited

Loading

rbargholz Sep 15, 2021 •

edited

Loading