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

[CLI DX] Improve the 'x errors' message in the CLI #45713

Closed
orta opened this issue Sep 3, 2021 · 5 comments · Fixed by #45742
Closed

[CLI DX] Improve the 'x errors' message in the CLI #45713

orta opened this issue Sep 3, 2021 · 5 comments · Fixed by #45742
Labels
Effort: Casual Good issue if you're already used to contributing to the codebase. Harder than "good first issue". Experience Enhancement Noncontroversial enhancements Good First Issue Well scoped, documented and has the green light Help Wanted You can do this
Milestone

Comments

@orta
Copy link
Contributor

orta commented Sep 3, 2021

Suggestion

As of 4.4, a run of tsc looks like:

$ /Users/ortatherox/dev/typescript/repros/compilerSamples/node_modules/.bin/tsc
                                                                               
// ...                                                                         
                                                                               
Found 4 errors.                                                                

I propose instead we change this message in two key ways, depending on the number of files are raised with errors:

Single file

$ /Users/ortatherox/dev/typescript/repros/compilerSamples/node_modules/.bin/tsc
                                                                               
// ...                                                                         
                                                                               
Found 1 error in index.ts:4                                                    

Multi file

$ /Users/ortatherox/dev/typescript/repros/compilerSamples/node_modules/.bin/tsc
                                                                               
// ...                                                                         

Found 4 errors in 3 files.           
                                     
Errors  Files                        
                                     
     2  index.ts:2                   
     1  other.ts:4                   
     1  third.ts:8

Reasoning

For single files, it's a nice quick small dx improvement which lets you click directly to the file. For multi-files, it's a way to understand the entire scope of your compiler errors. Today to understand what files and where the general amount of errors come from you need to scan the entire text, and if that has scrolled off the terminal - then this is hard to do.

In both cases we only link to the file and line, having the character on the file isn't that useful at this point.

Exceptions

A PR should not include these changes in watch mode. We think there are third-party apps which rely on the text in watch mode, and we want to make sure we've found those and made sure they can handle the changes to the output. So, to not block PRs we request keeping the watch mode as the same output.

@orta orta added Help Wanted You can do this Experience Enhancement Noncontroversial enhancements Effort: Casual Good issue if you're already used to contributing to the codebase. Harder than "good first issue". Good First Issue Well scoped, documented and has the green light labels Sep 3, 2021
@rbargholz
Copy link
Contributor

I'd be interested to give this a shot if it hasn't been picked up yet. This would be my first foray into the code so I'll look into it as well first.

@orta
Copy link
Contributor Author

orta commented Sep 4, 2021

Sure, give it a shot there's no need to try and claim the issue - until there's a solid WIP PR it's open for anyone to give it a go.

rbargholz added a commit to rbargholz/TypeScript that referenced this issue Sep 5, 2021
@rbargholz
Copy link
Contributor

@orta Apologies for my new-ness, may I ask about running tests? I've implemented the first part of this ticket (I am yet to implement a breakdown of the errors), but multiple files and single files are handled. When running tests I appear to have a number of them failing with this kind of error:

  15)

         unittests:: tsbuild:: exitCodeOnBogusFile:: test exit code
           tsc -b bogus.json exitCodeOnBogusFile:: test exit code
             exitCodeOnBogusFile
               test exit code
                 "before all" hook for "Generates files matching the baseline":
     Error: Debug Failure.
      at C:\Users\rbarg\dev\TypeScript\src\compiler\utilities.ts:5850:79
      at String.replace (<anonymous>)
      at formatStringFromArgs (src\compiler\utilities.ts:5850:21)
      at Object.createCompilerDiagnostic (src\compiler\utilities.ts:5971:20)
      at Object.getErrorSummaryText (src\compiler\watch.ts:135:13)
      at Object.reportErrorSummary (src\executeCommandLine\executeCommandLine.ts:752:53)
      at reportErrorSummary (src\compiler\tsbuildPublic.ts:2028:24)
      at build (src\compiler\tsbuildPublic.ts:1696:9)
      at Object.build (src\compiler\tsbuildPublic.ts:1947:86)
      at performBuild (src\executeCommandLine\executeCommandLine.ts:745:75)
      at Object.executeCommandLine (src\executeCommandLine\executeCommandLine.ts:647:24)
      at tscCompile (src\testRunner\unittests\tsc\helpers.ts:113:9)
      at Context.<anonymous> (src\testRunner\unittests\tsc\helpers.ts:163:31)
      at processImmediate (internal/timers.js:464:21)

Is this common?

rbargholz added a commit to rbargholz/TypeScript that referenced this issue Sep 5, 2021
rbargholz added a commit to rbargholz/TypeScript that referenced this issue Sep 6, 2021
@rbargholz
Copy link
Contributor

@orta Apologies for my new-ness, may I ask about running tests? I've implemented the first part of this ticket (I am yet to implement a breakdown of the errors), but multiple files and single files are handled. When running tests I appear to have a number of them failing with this kind of error:

  15)

         unittests:: tsbuild:: exitCodeOnBogusFile:: test exit code
           tsc -b bogus.json exitCodeOnBogusFile:: test exit code
             exitCodeOnBogusFile
               test exit code
                 "before all" hook for "Generates files matching the baseline":
     Error: Debug Failure.
      at C:\Users\rbarg\dev\TypeScript\src\compiler\utilities.ts:5850:79
      at String.replace (<anonymous>)
      at formatStringFromArgs (src\compiler\utilities.ts:5850:21)
      at Object.createCompilerDiagnostic (src\compiler\utilities.ts:5971:20)
      at Object.getErrorSummaryText (src\compiler\watch.ts:135:13)
      at Object.reportErrorSummary (src\executeCommandLine\executeCommandLine.ts:752:53)
      at reportErrorSummary (src\compiler\tsbuildPublic.ts:2028:24)
      at build (src\compiler\tsbuildPublic.ts:1696:9)
      at Object.build (src\compiler\tsbuildPublic.ts:1947:86)
      at performBuild (src\executeCommandLine\executeCommandLine.ts:745:75)
      at Object.executeCommandLine (src\executeCommandLine\executeCommandLine.ts:647:24)
      at tscCompile (src\testRunner\unittests\tsc\helpers.ts:113:9)
      at Context.<anonymous> (src\testRunner\unittests\tsc\helpers.ts:163:31)
      at processImmediate (internal/timers.js:464:21)

Is this common?

This has come good, I found it was due to some baselines that didn't match, or were causing some issues.

@orta
Copy link
Contributor Author

orta commented Sep 6, 2021

Ace, sorry just read all this - looking good so far 👍🏻

rbargholz added a commit to rbargholz/TypeScript that referenced this issue Sep 13, 2021
rbargholz added a commit to rbargholz/TypeScript that referenced this issue Sep 14, 2021
@sandersn sandersn added this to the Backlog milestone Sep 15, 2021
orta added a commit that referenced this issue Dec 7, 2021
* Improve error report summaries (#45713)

* fixup! Improve error report summaries (#45713)

* fixup! fixup! Improve error report summaries (#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 <git@orta.io>
Co-authored-by: Orta Therox <ortam@microsoft.com>
mprobst pushed a commit to mprobst/TypeScript that referenced this issue Jan 10, 2022
* 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 <git@orta.io>
Co-authored-by: Orta Therox <ortam@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Effort: Casual Good issue if you're already used to contributing to the codebase. Harder than "good first issue". Experience Enhancement Noncontroversial enhancements Good First Issue Well scoped, documented and has the green light Help Wanted You can do this
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants