## Summary This PR fixes #7352 by exposing the `show_fix_diff` option used in our snapshot tests in the CLI. As the issue suggests, we plan to make this the default output format in the future, so this is added to the `full` output format in preview for now. This turned out to be pretty straightforward. I just used our existing `Applicability` settings to determine whether or not to print the diff. The snapshot differences are because we now set `Applicability::DisplayOnly` for our snapshot tests. This `Applicability` is also used to determine whether or not the fix icon (`[*]`) is rendered, so this is now shown for display-only fixes in our snapshots. This was already the case previously, but we were only setting `Applicability::Unsafe` in these tests and ignoring the `Applicability` when rendering fix diffs. CLI users can't enable display-only fixes, so this is only a test change for now, but this should work smoothly if we decide to expose a `--display-only-fixes` flag or similar in the future. I also deleted the `PrinterFlags::SHOW_FIX_DIFF` flag. This was completely unused before, and it seemed less confusing just to delete it than to enable it in the right place and check it along with the `OutputFormat` and `preview`. ## Test Plan I only added one CLI test for now. I'm kind of assuming that we have decent coverage of the cases where this shouldn't be firing, especially the `output_format` CLI test, which shows that this definitely doesn't affect non-preview `full` output. I'm happy to add more tests with different combinations of options, if we're worried about any in particular. I did try `--diff` and `--preview` and a few other combinations manually. And here's a screenshot using our trusty UP049 example from the design discussion confirming that all the colors and other formatting still look as expected: <img width="786" height="629" alt="image" src="https://github.com/user-attachments/assets/94e408bc-af7b-4573-b546-a5ceac2620f2" /> And one with an unsafe fix to see the footer: <img width="782" height="367" alt="image" src="https://github.com/user-attachments/assets/bbb29e47-310b-4293-b2c2-cc7aee3baff4" /> ## Related issues and PR - https://github.com/astral-sh/ruff/issues/7352 - https://github.com/astral-sh/ruff/pull/12595 - https://github.com/astral-sh/ruff/issues/12598 - https://github.com/astral-sh/ruff/issues/12599 - https://github.com/astral-sh/ruff/issues/12600 I think we could probably close all of these issues now. I think we've either resolved or avoided most of them, and if we encounter them again with the new output format, it would probably make sense to open new ones anyway.
200 lines
7.3 KiB
Rust
200 lines
7.3 KiB
Rust
use crate::diagnostic::{
|
|
Diagnostic, DisplayDiagnosticConfig, Severity,
|
|
stylesheet::{DiagnosticStylesheet, fmt_styled},
|
|
};
|
|
|
|
use super::FileResolver;
|
|
|
|
pub(super) struct ConciseRenderer<'a> {
|
|
resolver: &'a dyn FileResolver,
|
|
config: &'a DisplayDiagnosticConfig,
|
|
}
|
|
|
|
impl<'a> ConciseRenderer<'a> {
|
|
pub(super) fn new(resolver: &'a dyn FileResolver, config: &'a DisplayDiagnosticConfig) -> Self {
|
|
Self { resolver, config }
|
|
}
|
|
|
|
pub(super) fn render(
|
|
&self,
|
|
f: &mut std::fmt::Formatter,
|
|
diagnostics: &[Diagnostic],
|
|
) -> std::fmt::Result {
|
|
let stylesheet = if self.config.color {
|
|
DiagnosticStylesheet::styled()
|
|
} else {
|
|
DiagnosticStylesheet::plain()
|
|
};
|
|
|
|
let sep = fmt_styled(":", stylesheet.separator);
|
|
for diag in diagnostics {
|
|
if let Some(span) = diag.primary_span() {
|
|
write!(
|
|
f,
|
|
"{path}",
|
|
path = fmt_styled(
|
|
span.file().relative_path(self.resolver).to_string_lossy(),
|
|
stylesheet.emphasis
|
|
)
|
|
)?;
|
|
if let Some(range) = span.range() {
|
|
let diagnostic_source = span.file().diagnostic_source(self.resolver);
|
|
let start = diagnostic_source
|
|
.as_source_code()
|
|
.line_column(range.start());
|
|
|
|
if let Some(notebook_index) = self.resolver.notebook_index(span.file()) {
|
|
write!(
|
|
f,
|
|
"{sep}cell {cell}{sep}{line}{sep}{col}",
|
|
cell = notebook_index.cell(start.line).unwrap_or_default(),
|
|
line = notebook_index.cell_row(start.line).unwrap_or_default(),
|
|
col = start.column,
|
|
)?;
|
|
} else {
|
|
write!(
|
|
f,
|
|
"{sep}{line}{sep}{col}",
|
|
line = start.line,
|
|
col = start.column,
|
|
)?;
|
|
}
|
|
}
|
|
write!(f, "{sep} ")?;
|
|
}
|
|
if self.config.hide_severity {
|
|
if let Some(code) = diag.secondary_code() {
|
|
write!(
|
|
f,
|
|
"{code} ",
|
|
code = fmt_styled(code, stylesheet.secondary_code)
|
|
)?;
|
|
} else {
|
|
write!(
|
|
f,
|
|
"{id}: ",
|
|
id = fmt_styled(diag.inner.id.as_str(), stylesheet.secondary_code)
|
|
)?;
|
|
}
|
|
if self.config.show_fix_status {
|
|
// Do not display an indicator for inapplicable fixes
|
|
if diag.has_applicable_fix(self.config) {
|
|
write!(f, "[{fix}] ", fix = fmt_styled("*", stylesheet.separator))?;
|
|
}
|
|
}
|
|
} else {
|
|
let (severity, severity_style) = match diag.severity() {
|
|
Severity::Info => ("info", stylesheet.info),
|
|
Severity::Warning => ("warning", stylesheet.warning),
|
|
Severity::Error => ("error", stylesheet.error),
|
|
Severity::Fatal => ("fatal", stylesheet.error),
|
|
};
|
|
write!(
|
|
f,
|
|
"{severity}[{id}] ",
|
|
severity = fmt_styled(severity, severity_style),
|
|
id = fmt_styled(diag.id(), stylesheet.emphasis)
|
|
)?;
|
|
}
|
|
|
|
writeln!(f, "{message}", message = diag.concise_message())?;
|
|
}
|
|
|
|
Ok(())
|
|
}
|
|
}
|
|
|
|
#[cfg(test)]
|
|
mod tests {
|
|
use ruff_diagnostics::Applicability;
|
|
|
|
use crate::diagnostic::{
|
|
DiagnosticFormat,
|
|
render::tests::{
|
|
TestEnvironment, create_diagnostics, create_notebook_diagnostics,
|
|
create_syntax_error_diagnostics,
|
|
},
|
|
};
|
|
|
|
#[test]
|
|
fn output() {
|
|
let (env, diagnostics) = create_diagnostics(DiagnosticFormat::Concise);
|
|
insta::assert_snapshot!(env.render_diagnostics(&diagnostics), @r"
|
|
fib.py:1:8: error[unused-import] `os` imported but unused
|
|
fib.py:6:5: error[unused-variable] Local variable `x` is assigned to but never used
|
|
undef.py:1:4: error[undefined-name] Undefined name `a`
|
|
");
|
|
}
|
|
|
|
#[test]
|
|
fn show_fixes() {
|
|
let (mut env, diagnostics) = create_diagnostics(DiagnosticFormat::Concise);
|
|
env.hide_severity(true);
|
|
env.show_fix_status(true);
|
|
env.fix_applicability(Applicability::DisplayOnly);
|
|
insta::assert_snapshot!(env.render_diagnostics(&diagnostics), @r"
|
|
fib.py:1:8: F401 [*] `os` imported but unused
|
|
fib.py:6:5: F841 [*] Local variable `x` is assigned to but never used
|
|
undef.py:1:4: F821 Undefined name `a`
|
|
");
|
|
}
|
|
|
|
#[test]
|
|
fn show_fixes_preview() {
|
|
let (mut env, diagnostics) = create_diagnostics(DiagnosticFormat::Concise);
|
|
env.hide_severity(true);
|
|
env.show_fix_status(true);
|
|
env.fix_applicability(Applicability::DisplayOnly);
|
|
env.preview(true);
|
|
insta::assert_snapshot!(env.render_diagnostics(&diagnostics), @r"
|
|
fib.py:1:8: F401 [*] `os` imported but unused
|
|
fib.py:6:5: F841 [*] Local variable `x` is assigned to but never used
|
|
undef.py:1:4: F821 Undefined name `a`
|
|
");
|
|
}
|
|
|
|
#[test]
|
|
fn show_fixes_syntax_errors() {
|
|
let (mut env, diagnostics) = create_syntax_error_diagnostics(DiagnosticFormat::Concise);
|
|
env.hide_severity(true);
|
|
env.show_fix_status(true);
|
|
env.fix_applicability(Applicability::DisplayOnly);
|
|
insta::assert_snapshot!(env.render_diagnostics(&diagnostics), @r"
|
|
syntax_errors.py:1:15: invalid-syntax: Expected one or more symbol names after import
|
|
syntax_errors.py:3:12: invalid-syntax: Expected ')', found newline
|
|
");
|
|
}
|
|
|
|
#[test]
|
|
fn syntax_errors() {
|
|
let (env, diagnostics) = create_syntax_error_diagnostics(DiagnosticFormat::Concise);
|
|
insta::assert_snapshot!(env.render_diagnostics(&diagnostics), @r"
|
|
syntax_errors.py:1:15: error[invalid-syntax] Expected one or more symbol names after import
|
|
syntax_errors.py:3:12: error[invalid-syntax] Expected ')', found newline
|
|
");
|
|
}
|
|
|
|
#[test]
|
|
fn notebook_output() {
|
|
let (env, diagnostics) = create_notebook_diagnostics(DiagnosticFormat::Concise);
|
|
insta::assert_snapshot!(env.render_diagnostics(&diagnostics), @r"
|
|
notebook.ipynb:cell 1:2:8: error[unused-import] `os` imported but unused
|
|
notebook.ipynb:cell 2:2:8: error[unused-import] `math` imported but unused
|
|
notebook.ipynb:cell 3:4:5: error[unused-variable] Local variable `x` is assigned to but never used
|
|
");
|
|
}
|
|
|
|
#[test]
|
|
fn missing_file() {
|
|
let mut env = TestEnvironment::new();
|
|
env.format(DiagnosticFormat::Concise);
|
|
|
|
let diag = env.err().build();
|
|
|
|
insta::assert_snapshot!(
|
|
env.render(&diag),
|
|
@"error[test-diagnostic] main diagnostic message",
|
|
);
|
|
}
|
|
}
|