Skip to content

Output list instead of table#5426

Open
fregante wants to merge 3 commits intomozilla:masterfrom
fregante:list-output
Open

Output list instead of table#5426
fregante wants to merge 3 commits intomozilla:masterfrom
fregante:list-output

Conversation

@fregante
Copy link
Copy Markdown
Contributor

Before

This isn't even the full table, it doesn't fit on my screen

Screenshot 3

After

Screenshot 2

Comment thread src/linter.js
@fregante
Copy link
Copy Markdown
Contributor Author

fregante commented Jan 3, 2025

@rpl @willdurand I believe the improvements here are evident from my screenshots:

  • the output is shorter
  • there's no duplicate explanation
    Screenshot 3
  • there's no bad wrapping of long text
    Screenshot 2
  • paths are always clickable
    Screenshot 4
  • row and col are part of the path, meaning IDEs can reach them in the same click
    Screenshot
  • the table implementation isn't even right because the error/warning tables are not aligned
    Screenshot

The only reason not to move forward with this is because it already works, albeit in this suboptimal fashion. Since I'm currently available to work on this, allow me to complete this change and merge this PR. I don’t really like keeping PRs open for years

Copy link
Copy Markdown
Member

@Rob--W Rob--W 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 improving the readability and dropping one dependency. We're willing to switch the output format.

Could you adjust the text and update the tests so that they pass?

Thanks for this, and apologies for the review delay!

Comment thread src/linter.js Outdated
@fregante
Copy link
Copy Markdown
Contributor Author

Great, let's see if I have time this month

@willdurand
Copy link
Copy Markdown
Member

I know we haven't been very responsive here. Should we attempt to change the output or should we drop the idea?

@fregante
Copy link
Copy Markdown
Contributor Author

This month I'll be a bit busy. If the changes are purely esthetic please take it over or assign Copilot to finish it up.

@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 16, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.79%. Comparing base (665acc3) to head (feda392).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5426      +/-   ##
==========================================
- Coverage   98.79%   98.79%   -0.01%     
==========================================
  Files          51       51              
  Lines        2905     2893      -12     
  Branches      895      897       +2     
==========================================
- Hits         2870     2858      -12     
  Misses         35       35              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@willdurand
Copy link
Copy Markdown
Member

This is obvious in the tests but it wasn't when I looked at the screenshots.. The description is gone. @fregante was that intended?

@fregante
Copy link
Copy Markdown
Contributor Author

fregante commented Feb 16, 2026

Most likely not intentional, it should appear just after the message as in the original order. In some cases it does seem to be identical to the message so it should likely be dropped there instead of repeating it.

@willdurand willdurand marked this pull request as ready for review February 18, 2026 11:01
@willdurand willdurand requested review from Rob--W and rpl February 18, 2026 11:01
@Rob--W
Copy link
Copy Markdown
Member

Rob--W commented Feb 26, 2026

@rpl @willdurand and I reviewed the result together.

Observed changes, explicitly approved:

  • Previously the text was wrapped. Now it is not. We are okay with it. If there is demand for the functionality (e.g. through a bug report in the future) we can reconsider at that time.

Requested changes to behavior:

  • Append (warning) / (error) (or prepend warning/error) to the error code so that it is more obvious whether a code is a warning/error when the user looks at a fragment of the output.
  • Remove indentation level (so that the error code and description does not have space at the left).
  • Make it more obvious that the file is the file where the error occurred.

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.

Output as list, not table

3 participants