mirror of
https://github.com/astral-sh/ruff.git
synced 2025-09-29 05:14:52 +00:00
Move full diagnostic rendering to ruff_db
(#19415)
## Summary This PR switches the `full` output format in Ruff over to use the rendering code in `ruff_db`. As proposed in the design doc, this involves a lot of changes to the snapshot output. I also had to comment out this assertion with a TODO to replace it after https://github.com/astral-sh/ruff/issues/19688 because many of Ruff's "file-level" annotations aren't actually file-level. They just happen to occur at the start of the file, especially in tests with very short snippets.529d81daca/crates/ruff_annotate_snippets/src/renderer/display_list.rs (L1204-L1208)
I broke up the snapshot commits at the end into several blocks, but I don't think it's enough to help with review. The first few (notebooks, syntax errors, and test rules) are small enough to look at, but I couldn't really think of other categories beyond that. I'm happy to break those up or pick out specific examples beyond what I have below, if that would help. The minimal code changes are in this [range](abd28f1e77
), with the snapshot commits following. Moving the `FullRenderer` and updating the `EmitterFlags` aren't strictly necessary either. I even dropped the renderer commit this morning but figured it made sense to keep it since we have the `full` module for tests. I don't feel strongly either way. ## Test Plan I did actually click through all 1700 snapshots individually instead of accepting them all at once, although I moved through them quickly. There are a few main categories: ### Lint diagnostics ```diff -unused.py:8:19: F401 [*] `pathlib` imported but unused +F401 [*] `pathlib` imported but unused + --> unused.py:8:19 | 7 | # Unused, _not_ marked as required (due to the alias). 8 | import pathlib as non_alias - | ^^^^^^^^^ F401 + | ^^^^^^^^^ 9 | 10 | # Unused, marked as required. | - = help: Remove unused import: `pathlib` +help: Remove unused import: `pathlib` ``` - The filename and line numbers are moved to the second line - The second noqa code next to the underline is removed ### Syntax errors These are much like the above. ```diff - -:1:16: invalid-syntax: Expected one or more symbol names after import + invalid-syntax: Expected one or more symbol names after import + --> -:1:16 | 1 | from foo import | ^ ``` One thing I noticed while reviewing some of these, but I don't think is strictly syntax-error-related, is that some of the new diagnostics have a little less context after the error. I don't think this is a problem, but it's one small discrepancy I hadn't noticed before. Here's a minor example: ```diff -syntax_errors.py:1:15: invalid-syntax: Expected one or more symbol names after import +invalid-syntax: Expected one or more symbol names after import + --> syntax_errors.py:1:15 | 1 | from os import | ^ 2 | 3 | if call(foo -4 | def bar(): | ``` And one of the biggest examples: ```diff -E30_syntax_error.py:18:11: invalid-syntax: Expected ')', found newline +invalid-syntax: Expected ')', found newline + --> E30_syntax_error.py:18:11 | 16 | pass 17 | 18 | foo = Foo( | ^ -19 | -20 | -21 | def top( | ``` Similarly, a few of the lint diagnostics showed that the cut indicator calculation for overly long lines is also slightly different, but I think that's okay too. ### Full-file diagnostics ```diff -comment.py:1:1: I002 [*] Missing required import: `from __future__ import annotations` +I002 [*] Missing required import: `from __future__ import annotations` +--> comment.py:1:1 +help: Insert required import: `from __future__ import annotations` + ``` As noted above, these will be much more rare after #19688 too. This case isn't a true full-file diagnostic and will render a snippet in the future, but you can see that we're now rendering the help message that would have been discarded before. In contrast, this is a true full-file diagnostic and should still look like this after #19688: ```diff -__init__.py:1:1: A005 Module `logging` shadows a Python standard-library module +A005 Module `logging` shadows a Python standard-library module +--> __init__.py:1:1 ``` ### Jupyter notebooks There's nothing particularly different about these, just showing off the cell index again. ```diff - Jupyter.ipynb:cell 3:1:7: F821 Undefined name `x` + F821 Undefined name `x` + --> Jupyter.ipynb:cell 3:1:7 | 1 | print(x) - | ^ F821 + | ^ | ```
This commit is contained in:
parent
8489816edc
commit
44755e6e86
1684 changed files with 46025 additions and 33951 deletions
|
@ -1201,11 +1201,16 @@ fn format_snippet<'m>(
|
|||
|
||||
let is_file_level = snippet.annotations.iter().any(|ann| ann.is_file_level);
|
||||
if is_file_level {
|
||||
assert!(
|
||||
snippet.source.is_empty(),
|
||||
"Non-empty file-level snippet that won't be rendered: {:?}",
|
||||
snippet.source
|
||||
);
|
||||
// TODO(brent) enable this assertion again once we set `is_file_level` for individual rules.
|
||||
// It's causing too many false positives currently when the default is to make any
|
||||
// annotation with a default range file-level. See
|
||||
// https://github.com/astral-sh/ruff/issues/19688.
|
||||
//
|
||||
// assert!(
|
||||
// snippet.source.is_empty(),
|
||||
// "Non-empty file-level snippet that won't be rendered: {:?}",
|
||||
// snippet.source
|
||||
// );
|
||||
let header = format_header(origin, main_range, &[], is_first, snippet.cell_index);
|
||||
return DisplaySet {
|
||||
display_lines: header.map_or_else(Vec::new, |header| vec![header]),
|
||||
|
|
Loading…
Add table
Add a link
Reference in a new issue