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

Collapse output to 20 items #160

Merged
merged 23 commits into from
Jan 22, 2025
Merged

Collapse output to 20 items #160

merged 23 commits into from
Jan 22, 2025

Conversation

daniilsapa
Copy link
Collaborator

@daniilsapa daniilsapa commented Jan 2, 2025

Closes #51

The solution is still raw and it's an early PR, so you can see the general approach and we can clarify some details.

  1. When I was working on this logic I came across a thought that we probably need to make the number we collapse diagnostics to configurable and introduce a new config object type for such things that configure Steiger itself (it is very likely that other options will appear pretty soon) and not severities/options for rules. It seems it's better to introduce such a new object type earlier, e.g. in the next minor update.
  2. Should we introduce severity to consideration when we collapse diagnostics? Looks like we need to count error first because it's the thing that breaks CIs or local checks. It would be quite annoying if you do not see an error because the quota is exceeded with warnings
  3. It looks like runRules from app.ts needs to return some Report interface, so it contains not just diagnostics but also some meta info about them. We need this to display the number of shown vs hidden diagnostics etc. other fields may appear in the future.

@daniilsapa daniilsapa requested a review from illright January 2, 2025 23:23
Copy link

changeset-bot bot commented Jan 2, 2025

🦋 Changeset detected

Latest commit: f3b4263

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@steiger/pretty-reporter Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@illright
Copy link
Member

illright commented Jan 3, 2025

Interesting considerations, thanks for the early update! I looked at the code and realized that I forgot to mention one important detail in the issue description.

I originally envisioned this as a purely cosmetic thing rather than a linter feature, and as such, I planned that this functionality would be added to the pretty reporter rather than the linter core. This way, all reporters get the same full diagnostics, but some reporters might choose to hide some.

The inspiration for this feature came from Biome, they also have a 20-diagnostic limit. Here's how they show it:
image

That's why I don't think we should make this thing configurable or even disable-able. If one wants to see all diagnostics, I think it's not because they want to read all of them individually. For mass-reading of diagnostics, the JSON reporter is probably better. I suggest we write "X diagnostics were hidden, run with --reporter json to see all diagnostics" or something.

The severity consideration is a very good point, I agree that we probably want to sort the errors first and then, if there's space left, print warnings too. If there's a rule that has produced only warnings, then we only print its diagnostics if all the errors have been printed.

@daniilsapa
Copy link
Collaborator Author

Oh, yeah, actually it makes sense to make it a cosmetic thing and move pretty-reporter. It would also make the implementation easier and the resulting codebase less complex. Do you like the general approach? If so, I will move the logic to pretty-reporter, add severity to consideration, and refactor it a bit.

@illright
Copy link
Member

illright commented Jan 3, 2025

Overall the code looks good, I only have minor concerns about over-abstracting — such as having an entire feature for this, and also making the quota a modifiable parameter. Seems like the code could be simpler by keeping it all in one function

@daniilsapa
Copy link
Collaborator Author

@illright
I've moved the solution to pretty-reporter. Also, you can see the output here:

Screenshot 2025-01-06 at 15 51 11

Let me know if you think we must rephrase/change it somehow

@daniilsapa
Copy link
Collaborator Author

The bundle size pipeline fails when the git branch contains slashes. I'll fix the problem as part of this issue

Screenshot 2025-01-06 at 16 51 03

Copy link
Member

@illright illright left a comment

Choose a reason for hiding this comment

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

Thanks for noticing the bundle size workflow issue, my mistake! I think we can replace everything that isn't /[0-9a-zA-Z-._]/ (ref) with - to overcome this.

The solution is good overall, but slightly hard to understand, I made some suggestions to improve clarity, let me know what you think

packages/pretty-reporter/src/index.ts Outdated Show resolved Hide resolved
packages/pretty-reporter/src/index.ts Outdated Show resolved Hide resolved
packages/pretty-reporter/src/collapse-diagnostics.ts Outdated Show resolved Hide resolved
packages/pretty-reporter/src/collapse-diagnostics.ts Outdated Show resolved Hide resolved
packages/pretty-reporter/src/collapse-diagnostics.ts Outdated Show resolved Hide resolved
packages/pretty-reporter/src/collapse-diagnostics.test.ts Outdated Show resolved Hide resolved
packages/pretty-reporter/src/collapse-diagnostics.test.ts Outdated Show resolved Hide resolved
@daniilsapa
Copy link
Collaborator Author

It looks like I fixed all the issues, please review when you have time

Copy link
Member

@illright illright left a comment

Choose a reason for hiding this comment

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

Yep, now the code is good! There is one potential issue in the workflow, and I figured I'd also throw in a little non-blocking suggestion as well

.github/workflows/bundle-size-trusted.yaml Show resolved Hide resolved
packages/pretty-reporter/src/collapse-diagnostics.ts Outdated Show resolved Hide resolved
Copy link
Member

@illright illright left a comment

Choose a reason for hiding this comment

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

Okay, this seems good to go!

Copy link

pkg-pr-new bot commented Jan 17, 2025

Open in Stackblitz

npm i https://pkg.pr.new/feature-sliced/steiger@160
npm i https://pkg.pr.new/feature-sliced/steiger/@feature-sliced/steiger-plugin@160
npm i https://pkg.pr.new/feature-sliced/steiger/@steiger/toolkit@160

commit: f3b4263

@daniilsapa daniilsapa merged commit 43d16ee into next Jan 22, 2025
12 checks passed
@daniilsapa daniilsapa deleted the feature/collapse-output branch January 22, 2025 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants