Skip to content

Conversation

@ntBre
Copy link
Contributor

@ntBre ntBre commented Aug 14, 2025

Summary

This is a quick implementation of the idea in #19900 (comment). Especially as we start using new diagnostic features, it seems like a good idea to keep an eye on the concise output format as well as the full. This PR just appends a second rendering of the diagnostics in our rule snapshots in the concise format. The concise message is typically reused in the other output formats too, so this kind of gives us a bit of coverage for all the output formats.

I noticed that some of our concise messages are still quite long. That might be another good class of diagnostic to use new features for. Some explanatory pieces might fit better as sub-diagnostics, now that we're not limited to a single line in the default case.

Adding these lines (and adding a let binding for emitter) is the only code change:

if !diagnostics.is_empty() {
output.extend(b"## Concise Output\n");
}
emitter
.with_show_source(false)
.with_show_fix_diff(false)
.emit(
&mut output,
diagnostics,
&EmitterContext::new(&FxHashMap::default()),
)
.unwrap();

Test Plan

Existing snapshots. Again, I clicked through these individually, but very, very quickly

Summary
--

This is a quick implementation of the idea in
#19900 (comment). Especially
as we start using new diagnostic features, it seems like a good idea to keep an
eye on the `concise` output format as well as the `full`. This PR just appends a
second rendering of the diagnostics in our rule snapshots in the `concise`
format. The concise message is typically reused in the other output formats too,
so this gives us a bit of coverage for all the output formats.

I noticed that some of our concise messages are still quite long. That might be
another good class of diagnostic to use new features for. Some explanatory
pieces might fit better as sub-diagnostics.

Test Plan
--

Existing snapshots. Again, I clicked through these individually, but very, very
quickly
@ntBre ntBre force-pushed the brent/concise-snapshots branch from 6b0b78f to 1fbb88f Compare August 14, 2025 14:00
@ntBre ntBre added the testing Related to testing Ruff itself label Aug 14, 2025
@github-actions
Copy link
Contributor

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@ntBre ntBre marked this pull request as ready for review August 14, 2025 14:11
@ntBre ntBre requested a review from AlexWaygood as a code owner August 14, 2025 14:11
@ntBre ntBre requested a review from MichaReiser August 14, 2025 14:11
@MichaReiser
Copy link
Member

Thanks for looking into this.

IMO, this contains too much redundant information and will only increase the noise when making changes to our tests. That's why I'd prefer a more granular opt in to testing the concise output

@AlexWaygood AlexWaygood removed their request for review August 18, 2025 07:53
@ntBre ntBre closed this Aug 18, 2025
@ntBre ntBre deleted the brent/concise-snapshots branch August 18, 2025 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testing Related to testing Ruff itself

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants