Skip to content

fix(45713) Improve error report summaries #45742

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.

Sign up for GitHub

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

Merged
orta merged 6 commits into from
Dec 7, 2021
Merged

fix(45713) Improve error report summaries #45742

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0638417
Improve error report summaries (#45713)
rbargholz Sep 5, 2021
08906dd
fixup! Improve error report summaries (#45713)
rbargholz Sep 13, 2021
1af84d2
fixup! fixup! Improve error report summaries (#45713)
rbargholz Sep 14, 2021
f543c40
Adds support for handling localization renaming the 'files' header du…
orta Oct 1, 2021
b2988e7
Merge branch 'main' into improved_error_summary
Oct 1, 2021
db56f9f
fixup! Adds support for handling localization renaming the 'files' he…
rbargholz Oct 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fixup! fixup! Improve error report summaries (#45713)
  • Loading branch information
@rbargholz
rbargholz committed Sep 14, 2021
commit 1af84d2978f2a976368f4d66487f0d599a4abbc3
15 changes: 10 additions & 5 deletions src/compiler/watch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,26 +148,31 @@ namespace ts {
distinctFileNamesWithLines[0]) :
createCompilerDiagnostic(
distinctFileNamesWithLines.length === 0 ?
Diagnostics.Found_0_errors :
Diagnostics.Found_0_errors :
distinctFileNamesWithLines.length === 1 ?
Diagnostics.Found_0_errors_in_1_file :
Diagnostics.Found_0_errors_in_1_files,
errorCount,
distinctFileNamesWithLines.length);
Copy link
Copy Markdown
Contributor

@orta orta Sep 14, 2021

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@rbargholz rbargholz Sep 15, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@rbargholz rbargholz Sep 15, 2021

Choose a reason for hiding this comment

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

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}`;

Copy link
Copy Markdown
Contributor

@orta orta Sep 15, 2021

Choose a reason for hiding this comment

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

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

return `${newLine}${flattenDiagnosticMessageText(d.messageText, newLine)}${newLine}${newLine}${errorCount > 1 ? createTabularErrorsDisplay(nonNilFiles) : ''}`;
return `${newLine}${flattenDiagnosticMessageText(d.messageText, newLine)}${newLine}${newLine}${errorCount > 1 ? createTabularErrorsDisplay(nonNilFiles) : ""}`;
}

function createTabularErrorsDisplay(filesInError: (ReportFileInError | undefined)[]) {
const BASE_LEFT_PADDING = 6;
let tabularData = "";
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";
Copy link
Copy Markdown
Member

@weswigham weswigham Sep 14, 2021

Choose a reason for hiding this comment

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

This probably needs to be a localized message~

Copy link
Copy Markdown
Contributor

@orta orta Sep 14, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@rbargholz rbargholz Sep 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor

@orta orta Sep 15, 2021

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor Author

@rbargholz rbargholz Sep 15, 2021

Choose a reason for hiding this comment

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

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

tabularData += headerRow;
distinctFiles.forEach(file => {
const errorCountForFile = countWhere(filesInError, fileInError => fileInError!.fileName === file!.fileName);
tabularData += ` ${errorCountForFile} ${file!.fileName}:${file!.line}\n`;
})
const errorCountDigitsLength = Math.log(errorCountForFile) * Math.LOG10E + 1 | 0;
Copy link
Copy Markdown
Contributor

@orta orta Sep 14, 2021

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@rbargholz rbargholz Sep 15, 2021

Choose a reason for hiding this comment

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

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

const leftPadding = errorCountDigitsLength < BASE_LEFT_PADDING ?
" ".repeat(BASE_LEFT_PADDING - errorCountDigitsLength)
: "";
tabularData += `${leftPadding}${errorCountForFile} ${file!.fileName}:${file!.line}\n`;
});

return tabularData;
}
Expand Down