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

📎 Implement biome lint --summary #57

Closed
Conaclos opened this issue Aug 23, 2023 · 21 comments · Fixed by #2742
Closed

📎 Implement biome lint --summary #57

Conaclos opened this issue Aug 23, 2023 · 21 comments · Fixed by #2742
Assignees
Labels
A-CLI Area: CLI A-Linter Area: linter S-Help-wanted Status: you're familiar with the code base and want to help the project

Comments

@Conaclos
Copy link
Member

Conaclos commented Aug 23, 2023

Description

See rome#4747 for some context.

biome lint --summary should print a summary which allows figuring out which rule raised diagnostics and how many.

$ biome lint --summary src

Rule name                          Diagnostics
nursery/useNamingConvention        2509
style/useSingleCaseStatement       2000
...

Checked 657 file(s) in 3s
Skipped 2 file(s)

The summary should avoid unnecessary works (no need to call diagnostic and action on the rules).

We could suggest using --summary when the number of diagnostics exceeds max-diagnostics:

$ rome lint src
...
src/services/types.ts:1121:5 lint/nursery/useNamingConvention ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  ⚠ This property name should be in camelCase.
  
  > 1121 │     InsertSpaceAfterConstructor?: boolean;
         │     ^^^^^^^^^^^^^^^^^^^^^^^^^^
  
  ℹ The name could be renamed to `insertSpaceAfterConstructor`.


The number of diagnostics exceeds the number allowed by Biome.
Diagnostics not shown: 7564.
Checked 657 file(s) in 3s
Skipped 2 file(s)

Run `biome lint --summary src` to print a summary of the rules that emitted diagnistics.
@ematipico
Copy link
Member

How would someone understand which diagnostics are errors and which ones are warnings?

@Conaclos
Copy link
Member Author

Conaclos commented Sep 3, 2023

The severity is set at the rule level? It could infer which one is an error using the rule name? We could also use a distinct color in the summary? For instance coloring the number f diagnostics in red for errors and orange for warnings.

@ematipico
Copy link
Member

ematipico commented Sep 3, 2023

The severity of the rules is set at configuration level.

I was trying to figure out if we can come up with a different output, because the proposed one doesn't distinguish between errors and warnings.

So, we should first be on the same page. Diagnostic is a generic term, while error/warning is a specific term.

Colours is a good suggestion, but it won't work in terminals that don't support colors, and in cases where users might need to scrap the output of the CLI.

@Conaclos Conaclos added the S-Help-wanted Status: you're familiar with the code base and want to help the project label Sep 3, 2023
@ematipico ematipico self-assigned this Sep 21, 2023
@ematipico
Copy link
Member

@Conaclos question:

Should --summary keep the log that says "Checked 11 file(s) is 4ms"?

@Conaclos
Copy link
Member Author

Conaclos commented Sep 21, 2023

Should --summary keep the log that says "Checked 11 file(s) is 4ms"?

I don't see any issue to keep it. I added it in the example.

@Conaclos
Copy link
Member Author

Conaclos commented Sep 21, 2023

By the way, I wonder if we should propose a common feature for both the summary and the structured output proposals. Instead of providing a --summary flag, we could have a --format suumary. In this case, we should omit the header and lines such as Checked 11 file(s) is 4ms. The output could be tab-or space- separated (this composes well with UNIX programs).

For example:

$ biome lint --format=summary src
nursery/useNamingConvention 2509
style/useSingleCaseStatement 2000
...

@ematipico
Copy link
Member

What do you think about --report instead?

In the future, I can see something like this:

biome check --report=summary,json

And Biome will emit:

  • a summary to stderr
  • json to stdout

And even

biome check --report=summary,json,junit --report-to-file

And Biome will:

  • a summary to stderr
  • json report to file
  • junit report to file

@Conaclos
Copy link
Member Author

Conaclos commented Sep 21, 2023

What do you think about --report instead?

I think it is a better name. --report or --reporter? Not sure.

Have you some use case for allowing several reporters at the same time?

I am not fond of the mix of behavior (reporting on stderr / stdout / file). I think we should choose between stderr or stdout? Maybe stdout?

For a first version, I think we can skip emitting a file. We could later add an option such as --report-file and guess the reporter type based on the file extension (this could not apply for summary, a user could be explicit using the --report option).

@ematipico
Copy link
Member

ematipico commented Sep 21, 2023

I am not fond of the mix of behavior (reporting on stderr / stdout / file). I think we should choose between stderr or stdout? Maybe stdout?

stderr is used for printing human readable messages, and stdout is used to printing machine readable messages.

We can remove the file for now, it was just a future idea and not related to this issue. Although, reporting on both stream is actually a thing.

@Conaclos
Copy link
Member Author

stderr is used for printing human readable messages, and stdout is used to printing machine readable messages.

You are right.
Even if summary is human-readable (at some extent), it should also be machine-readable.
So, I think we can stick for stdout :)

@e965
Copy link

e965 commented Dec 27, 2023

Saving the report would be a very cool feature! We could organize annotations in our repository, like we do now with the eslint report and this action https://github.com/ataylorme/eslint-annotate-action - it's very convenient

@hverlin
Copy link

hverlin commented Mar 30, 2024

Is there a plan to eventually have HTML reporter? (like https://eslint.org/docs/latest/use/formatters/#html) or supporting things like https://www.npmjs.com/package/eslint-formatter-gitlab (to be used on GitLab)

@ematipico ematipico self-assigned this May 6, 2024
@ematipico
Copy link
Member

Is there a plan to eventually have HTML reporter? (like eslint.org/docs/latest/use/formatters/#html) or supporting things like npmjs.com/package/eslint-formatter-gitlab (to be used on GitLab)

@hverlin Please use this github discussion instead, or create a new one. We usually use issues for technical discussions around the task at hand.

@ematipico
Copy link
Member

What do you think of this UI @Conaclos ?

     Running `target/debug/biome check './packages/@biomejs/js-api/main.ts' './packages/@biomejs/js-api/main2.ts' --reporter=summary --max-diagnostics=100`
Summarised report of diagnostics by file.

▶ ./packages/@biomejs/js-api/main.ts
     The file isn't formatted.

     Some lint rules were triggered

     Rule Name                                                         Diagnostics
     lint/suspicious/noImplicitAnyLet                                  6
     lint/suspicious/noDoubleEquals                                    3
     lint/suspicious/noRedeclare                                       5
     lint/suspicious/noDebugger                                        11

▶ ./packages/@biomejs/js-api/main2.ts
     The file isn't formatted.

     Some lint rules were triggered

     Rule Name                                                         Diagnostics
     lint/suspicious/noImplicitAnyLet                                  6
     lint/suspicious/noDoubleEquals                                    3
     lint/suspicious/noRedeclare                                       5
     lint/suspicious/noDebugger                                        11


Checked 2 files in 25ms. No fixes needed.
Found 54 errors.
check ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  ✖ Some errors were emitted while running checks.

Screenshot 2024-05-06 at 11 11 55

@ematipico
Copy link
Member

The word "Diagnostics" is dimmed, a bug in my code. I plan to rendering as "Rule Name"

@Conaclos
Copy link
Member Author

Conaclos commented May 6, 2024

Pretty nice :) Actually I was thinking of a general summary. However, I can see the interest for a file-based summary. Maybe we could add a general summary at the end?

Also:

  • I think we should remove Some lint rules were triggered because doesn't add so much value. We could rename Rule Name to Triggered lint rule.
  • I could remove the underline (this draws too much attention).
  • We could reduce a bit the spacing between the two columns
  • The file isn't formatted. and The file is formatted. are hard to distinguish. I am not sure how this can be improved.
  • Is it normal to get an error at the end?

@ematipico
Copy link
Member

ematipico commented May 6, 2024

Maybe we could add a general summary at the end?

Can you be more specific?

Is it normal to get an error at the end?

Yes? There are errors. The rules that were triggered are recommended, and the documents aren't formatted. The screenshot shows that I run biome check

However, I can see the interest for a file-based summary.

Yes, because it was requested in some other discussion. Also, this should work for other commands that aren't necessary just linting.

@mizdra
Copy link

mizdra commented May 6, 2024

I am the author of eslint-interactive. It is a tool that assists in correcting a large number of errors in ESLint. I would like to share my knowledge with you to help you resolve this discussion.

image

When eslint reports many errors, there are usually errors in many files. Therefore, if we summarize the errors by file, the output could be huge. It is not a good idea to summarize errors by file. Also, users often want to turn off rules that produce many errors. I think it would be nice to be able to display a summary for each rule to assist with this.

eslint-interactive also outputs the number of fixable problems. It would help users to identify rules that can be easily fixed with eslint --fix. In Biome, I think it would be good to output the number of safe fixes and unsafe fixes.

@Conaclos
Copy link
Member Author

Conaclos commented May 6, 2024

Can you be more specific?

▶ ./packages/@biomejs/js-api/main.ts
  The file isn't formatted.

  Triggered lint rule                                   Diagnostics
  lint/suspicious/noImplicitAnyLet                      3

▶ ./packages/@biomejs/js-api/main2.ts
  The file isn't formatted.

  Triggered lint rule                                   Diagnostics
  lint/suspicious/noImplicitAnyLet                      2
  lint/suspicious/noDoubleEquals                        3

▶ General summary
  Some files arn't formatted.

  Triggered lint rule                                   Diagnostics
  lint/suspicious/noImplicitAnyLet                      5
  lint/suspicious/noDoubleEquals                        3

When eslint reports many errors, there are usually errors in many files. Therefore, if we summarize the errors by file, the output could be huge. It is not a good idea to summarize errors by file. Also, users often want to turn off rules that produce many errors. I think it would be nice to be able to display a summary for each rule to assist with this.

Yes this was my first intention for the summary reporter: displaying how much a rule is triggered to assist the dev in enabling/disabling rules.

However, Biome is not currently able to output a summary for every file, this is what ESlint is doing by default. Maybe we should provide two reporters? summary and file-summary?

@ematipico
Copy link
Member

I think something is missing here. The idea is to have --summary available for each command (check does linting too), so other than linting we should think about the other commands and how to report them.

I think there's too much focus on the "lint rule", but the focus should be on diagnostics.

@ematipico
Copy link
Member

ematipico commented May 16, 2024

OK, I believe I landed something nice to see and informative, and that makes sense.

Formatter and Organize imports report the files that require changes, while linting groups everything by diagnostic category. We also break down errors, warnings, and information (this is for the near future).

The reason why we break down errors/warnings/infos is because a rule can be downgraded in different files using overrides, so we need to take that into account.

Screenshot 2024-05-16 at 09 26 14

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CLI Area: CLI A-Linter Area: linter S-Help-wanted Status: you're familiar with the code base and want to help the project
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants