Simplify Program configuration for GitHub output#20420
Simplify Program configuration for GitHub output#20420ntBre merged 4 commits intobrent/ty-githubfrom
Program configuration for GitHub output#20420Conversation
My initial idea here was to make each `render` method take a `&mut dyn std::io::Write` instead, but there are a few places where we use the `DisplayDiagnostic` `Display` implementation outside of rendering to stdout. This seemed like a decent way to preserve the `Display` functionality but also enable writing directly to an `io::Write` as well. revert non-github rendering changes
CodSpeed Instrumentation Performance ReportMerging #20420 will degrade performances by 12.28%Comparing Summary
Benchmarks breakdown
|
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| SIM910 | 6 | 0 | 6 | 0 | 0 |
|
I'm pretty sure the ecosystem and benchmark results are just a quirk of the base branch, but I'll keep an eye on that for sure. |
ntBre
requested review from
MichaReiser,
carljm,
dcreager and
sharkdp
as code owners
| diagnostics: &[Diagnostic], | ||
| ) -> std::fmt::Result { | ||
| for diagnostic in diagnostics { | ||
| impl std::fmt::Display for GithubRenderer<'_> { |
There was a problem hiding this comment.
It wasn't clear to me from the summary what the motivation was for implementing Display for Renderer but, after playing with the code, the issue seems to be that ty uses an std::fmt::Write whereas Ruff uses an std::io::Write.
I do find it odd that a Renderer can be displayed, especially because its display implementation shows the diagnostic and not the renderer.
- Introduce a
Renderertrait that has a singlerendermethod that takes astd::fmt::Formatterand a&[Diagnostic]and create a newDisplayDiagnostics<R: Renderer>that implementsDisplayby forwarding to the wrapped renderer. The main challenge with this is that we already have aDisplayDiagnosticstype inruff_db. My opinion is still that we should delete the oldDisplayDiagnosticstype and moveDiagnosticFormatout ofruff_db, as which formats are supported is a concern of ty/ruff. But this is a bit a larger refactor (but I think it would be worth it because it also helps clarify which program supports whichDiagnosticFormats). - Rename
GithubRenderertoGithubDiagnostics. That's obviously a much simpler change but it's a bit more awkward because it no longer fits into therendermodule.
For this PR, I think what I'd do is to do a mix of 1 and 2 by:
- Restore the
rendermethod onGithubRenderer - Introduce a new
DisplayGithubDiagnosticstype which takes a renderer and diagnostics as input and implementsDisplay. We can use this type in places where we need to write to astd::io::Write
There was a problem hiding this comment.
Ah yeah, sorry that wasn't clear. Yes, Ruff's Emitters take a &mut dyn std::io::Write, but DisplayDiagnostics and the Renderers in ty operate via std::fmt::Display and std::fmt::Formatters. As I mentioned on #20358, I thought the best fix was moving DisplayDiagnostics and the renderers to use std::io::Write too, but there were a few places where this wasn't an easy switch. These are two of the less important cases since they're in tests:
ruff/crates/ruff_db/src/diagnostic/render.rs
Lines 2749 to 2760 in aa63c24
This seemed like the more painful case that made me rethink the approach and try implementing Display for each renderer:
ruff/crates/ty_project/src/metadata/options.rs
Lines 1465 to 1472 in 34ba738
I just tried fixing that one and more keep popping up, so I think your suggestions sound very helpful. Thank you!
2 participants
Summary
This is a follow-up to #20358 to avoid adding a new
DisplayDiagnosticConfigoption (with a surprising default value) just to print the program name for GitHub diagnostics.Initially I expected this to be a larger refactor including making the various
Renderer::rendermethods take astd::io::Writeinstead of astd::fmt::Formatterto make them easier to call directly in Ruff. However, this was a bit more painful than expected because we use theDisplayimplementation ofDisplayDiagnosticsfor more than writing directly to stdout. ImplementingDisplayon the individual renderers seemed like a smaller step in this direction.At this point it might make sense just to merge this into #20358. Alternatively, I could apply the same transformation to the other affected renderers. For example, the JSON and JSON-lines formats only use their
configfields for thepreviewsetting. Or we could update all of the renderers and call them directly in Ruff instead of assembling a config at all. I just wanted to make sure this approach looked reasonable before applying it to everything.Test Plan
Existing tests