[red-knot] ruff_db: make diagnostic rendering prettier #15856
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,14 +28,21 @@ fn config_override() -> anyhow::Result<()> { | |
| ), | ||
| ])?; | ||
|
|
||
| assert_cmd_snapshot!(case.command(), @r###" | ||
| success: false | ||
| exit_code: 1 | ||
| ----- stdout ----- | ||
| error: lint:unresolved-attribute | ||
| --> <temp_dir>/test.py:5:7 | ||
| | | ||
| 4 | # Access `sys.last_exc` that was only added in Python 3.12 | ||
| 5 | print(sys.last_exc) | ||
| | ^^^^^^^^^^^^ Type `<module 'sys'>` has no attribute `last_exc` | ||
| | | ||
|
|
||
|
|
||
| ----- stderr ----- | ||
| "###); | ||
|
|
||
| assert_cmd_snapshot!(case.command().arg("--python-version").arg("3.12"), @r" | ||
| success: true | ||
|
|
@@ -91,14 +98,22 @@ fn cli_arguments_are_relative_to_the_current_directory() -> anyhow::Result<()> { | |
| ])?; | ||
|
|
||
| // Make sure that the CLI fails when the `libs` directory is not in the search path. | ||
| assert_cmd_snapshot!(case.command().current_dir(case.project_dir().join("child")), @r###" | ||
| success: false | ||
| exit_code: 1 | ||
| ----- stdout ----- | ||
| error: lint:unresolved-import | ||
| --> <temp_dir>/child/test.py:2:1 | ||
| | | ||
| 2 | from utils import add | ||
| | ^^^^^^^^^^^^^^^^^^^^^ Cannot resolve import `utils` | ||
| 3 | | ||
| 4 | stat = add(10, 15) | ||
| | | ||
|
|
||
|
|
||
| ----- stderr ----- | ||
| "###); | ||
|
|
||
| assert_cmd_snapshot!(case.command().current_dir(case.project_dir().join("child")).arg("--extra-search-path").arg("../libs"), @r" | ||
| success: true | ||
|
|
@@ -180,15 +195,31 @@ fn configuration_rule_severity() -> anyhow::Result<()> { | |
|
|
||
| // Assert that there's a possibly unresolved reference diagnostic | ||
| // and that division-by-zero has a severity of error by default. | ||
| assert_cmd_snapshot!(case.command(), @r###" | ||
| success: false | ||
| exit_code: 1 | ||
| ----- stdout ----- | ||
| error: lint:division-by-zero | ||
| --> <temp_dir>/test.py:2:5 | ||
| | | ||
| 2 | y = 4 / 0 | ||
| | ^^^^^ Cannot divide object of type `Literal[4]` by zero | ||
| 3 | | ||
| 4 | for a in range(0, y): | ||
| | | ||
|
|
||
| warning: lint:possibly-unresolved-reference | ||
| --> <temp_dir>/test.py:7:7 | ||
| | | ||
| 5 | x = a | ||
| 6 | | ||
| 7 | print(x) # possibly-unresolved-reference | ||
| | - Name `x` used when possibly not defined | ||
| | | ||
|
|
||
|
|
||
| ----- stderr ----- | ||
| "###); | ||
|
|
||
| case.write_file( | ||
| "pyproject.toml", | ||
|
|
@@ -199,14 +230,22 @@ fn configuration_rule_severity() -> anyhow::Result<()> { | |
| "#, | ||
| )?; | ||
|
|
||
| assert_cmd_snapshot!(case.command(), @r###" | ||
| success: false | ||
| exit_code: 1 | ||
| ----- stdout ----- | ||
| warning: lint:division-by-zero | ||
| --> <temp_dir>/test.py:2:5 | ||
| | | ||
| 2 | y = 4 / 0 | ||
| | ----- Cannot divide object of type `Literal[4]` by zero | ||
|
There was a problem hiding this comment. Ok really dumb question, but why does annotate-snippets choose There was a problem hiding this comment. Good question actually! It chooses a different mark based on the severity level: ruff/crates/ruff_annotate_snippets/src/renderer/display_list.rs Lines 716 to 723 in b0b8b06 Honestly kinda meh on this myself since it's somewhat non-obvious to me. There was a problem hiding this comment. I think it makes more sense once you have e.g. (multiple) "Info" and "Error" annotations in a single diagnostic. Example from my own project (thinking about it, the function parameter annotation on top here should probably also be "info", but anyway):
I kind of like how the informational parts are just underlined with There was a problem hiding this comment. Ah yeah that's a good point to consider too. I'll leave it as-is for now. This was just meant to get some decent rendering in. We can absolutely iterate on the exact format and what not. |
||
| 3 | | ||
| 4 | for a in range(0, y): | ||
| | | ||
|
|
||
|
|
||
| ----- stderr ----- | ||
| "###); | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
@@ -230,16 +269,42 @@ fn cli_rule_severity() -> anyhow::Result<()> { | |
|
|
||
| // Assert that there's a possibly unresolved reference diagnostic | ||
| // and that division-by-zero has a severity of error by default. | ||
| assert_cmd_snapshot!(case.command(), @r###" | ||
| success: false | ||
| exit_code: 1 | ||
| ----- stdout ----- | ||
| error: lint:unresolved-import | ||
| --> <temp_dir>/test.py:2:8 | ||
| | | ||
| 2 | import does_not_exit | ||
| | ^^^^^^^^^^^^^ Cannot resolve import `does_not_exit` | ||
| 3 | | ||
| 4 | y = 4 / 0 | ||
| | | ||
|
|
||
| error: lint:division-by-zero | ||
| --> <temp_dir>/test.py:4:5 | ||
| | | ||
| 2 | import does_not_exit | ||
| 3 | | ||
| 4 | y = 4 / 0 | ||
| | ^^^^^ Cannot divide object of type `Literal[4]` by zero | ||
| 5 | | ||
| 6 | for a in range(0, y): | ||
| | | ||
|
|
||
| warning: lint:possibly-unresolved-reference | ||
| --> <temp_dir>/test.py:9:7 | ||
| | | ||
| 7 | x = a | ||
| 8 | | ||
| 9 | print(x) # possibly-unresolved-reference | ||
| | - Name `x` used when possibly not defined | ||
| | | ||
|
|
||
|
|
||
| ----- stderr ----- | ||
| "###); | ||
|
|
||
| assert_cmd_snapshot!( | ||
| case | ||
|
|
@@ -250,15 +315,33 @@ fn cli_rule_severity() -> anyhow::Result<()> { | |
| .arg("division-by-zero") | ||
| .arg("--warn") | ||
| .arg("unresolved-import"), | ||
| @r###" | ||
| success: false | ||
| exit_code: 1 | ||
| ----- stdout ----- | ||
| warning: lint:unresolved-import | ||
| --> <temp_dir>/test.py:2:8 | ||
| | | ||
| 2 | import does_not_exit | ||
| | ------------- Cannot resolve import `does_not_exit` | ||
| 3 | | ||
| 4 | y = 4 / 0 | ||
| | | ||
|
|
||
| warning: lint:division-by-zero | ||
| --> <temp_dir>/test.py:4:5 | ||
| | | ||
| 2 | import does_not_exit | ||
| 3 | | ||
| 4 | y = 4 / 0 | ||
| | ----- Cannot divide object of type `Literal[4]` by zero | ||
| 5 | | ||
| 6 | for a in range(0, y): | ||
| | | ||
|
|
||
|
|
||
| ----- stderr ----- | ||
| "### | ||
| ); | ||
|
|
||
| Ok(()) | ||
|
|
@@ -282,15 +365,31 @@ fn cli_rule_severity_precedence() -> anyhow::Result<()> { | |
|
|
||
| // Assert that there's a possibly unresolved reference diagnostic | ||
| // and that division-by-zero has a severity of error by default. | ||
| assert_cmd_snapshot!(case.command(), @r###" | ||
| success: false | ||
| exit_code: 1 | ||
| ----- stdout ----- | ||
| error: lint:division-by-zero | ||
| --> <temp_dir>/test.py:2:5 | ||
| | | ||
| 2 | y = 4 / 0 | ||
| | ^^^^^ Cannot divide object of type `Literal[4]` by zero | ||
| 3 | | ||
| 4 | for a in range(0, y): | ||
| | | ||
|
|
||
| warning: lint:possibly-unresolved-reference | ||
| --> <temp_dir>/test.py:7:7 | ||
| | | ||
| 5 | x = a | ||
| 6 | | ||
| 7 | print(x) # possibly-unresolved-reference | ||
| | - Name `x` used when possibly not defined | ||
| | | ||
|
|
||
|
|
||
| ----- stderr ----- | ||
| "###); | ||
|
|
||
| assert_cmd_snapshot!( | ||
| case | ||
|
|
@@ -302,14 +401,22 @@ fn cli_rule_severity_precedence() -> anyhow::Result<()> { | |
| // Override the error severity with warning | ||
| .arg("--ignore") | ||
| .arg("possibly-unresolved-reference"), | ||
| @r###" | ||
| success: false | ||
| exit_code: 1 | ||
| ----- stdout ----- | ||
| warning: lint:division-by-zero | ||
| --> <temp_dir>/test.py:2:5 | ||
| | | ||
| 2 | y = 4 / 0 | ||
| | ----- Cannot divide object of type `Literal[4]` by zero | ||
| 3 | | ||
| 4 | for a in range(0, y): | ||
| | | ||
|
|
||
|
|
||
| ----- stderr ----- | ||
| "### | ||
| ); | ||
|
|
||
| Ok(()) | ||
|
|
@@ -329,14 +436,21 @@ fn configuration_unknown_rules() -> anyhow::Result<()> { | |
| ("test.py", "print(10)"), | ||
| ])?; | ||
|
|
||
| assert_cmd_snapshot!(case.command(), @r###" | ||
| success: false | ||
| exit_code: 1 | ||
| ----- stdout ----- | ||
| warning: unknown-rule | ||
| --> <temp_dir>/pyproject.toml:3:1 | ||
| | | ||
| 2 | [tool.knot.rules] | ||
| 3 | division-by-zer = "warn" # incorrect rule name | ||
| | --------------- Unknown lint rule `division-by-zer` | ||
| | | ||
|
|
||
|
|
||
| ----- stderr ----- | ||
| "###); | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
@@ -346,14 +460,15 @@ fn configuration_unknown_rules() -> anyhow::Result<()> { | |
| fn cli_unknown_rules() -> anyhow::Result<()> { | ||
| let case = TestCase::with_file("test.py", "print(10)")?; | ||
|
|
||
| assert_cmd_snapshot!(case.command().arg("--ignore").arg("division-by-zer"), @r###" | ||
| success: false | ||
| exit_code: 1 | ||
| ----- stdout ----- | ||
| warning: unknown-rule: Unknown lint rule `division-by-zer` | ||
|
|
||
|
|
||
| ----- stderr ----- | ||
| "###); | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This demonstrates that we should start capturing at least some full diagnostics in mdtests because highlighting the entire range here seems incorrect (compared to the
import utilscase below where it only highlights the name. I'll create an issueThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I agree. We need a fair bit of test coverage beyond what we have. I think a nice short-term target would be at least one test for every type of diagnostic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that we need a lot more tests that capture the full diagnostic output, but I'm not convinced this should necessarily be an mdtest feature (#15867 (comment))