Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[red-knot] Improve mdtest output #14213

Merged
merged 1 commit into from
Nov 11, 2024
Merged

Conversation

AlexWaygood
Copy link
Member

Summary

Helps with #13875. Changes made:

  • Paths given for the Markdown files are relative to the workspace root rather than the crate root. This enables you to copy-and-paste them straight from the terminal, and makes them clickable in more editors.
  • Use shorter titles for the bold/underlined section titles: just the filename for the first part of the title, rather than the path of the Markdown file relative to crates/red_knot_python_semantic/resources/mdtest. I didn't get rid of these entirely, because I believe it's possible to have a Markdown test file that doesn't have any # headings in it, which I believe would result in an empty title if we got rid of the path from the title entirely.
  • Reduce the indentation of the line-by-line error reports from 4 spaces to 2. I'm not wild about getting rid of these entirely for the reasons I gave in [red-knot] mdtest output improvements #13875 (comment), but I think it makes sense to reduce them a bit.

Previous output:

Screenshot

image

New output:

Screenshot

image

Diff to achieve these demo test failures:

--- a/crates/red_knot_python_semantic/resources/mdtest/comparison/identity_tests.md
+++ b/crates/red_knot_python_semantic/resources/mdtest/comparison/identity_tests.md
@@ -1,5 +1,7 @@
 # Identity tests
 
+## Foo
+
 ```py
 class A: ...
 
@@ -14,7 +16,7 @@ n2 = None
 
 o = get_object()
 
-reveal_type(a1 is a1)  # revealed: bool
+reveal_type(a1 is a1)  # revealed: str
 reveal_type(a1 is a2)  # revealed: bool
 
 reveal_type(n1 is n1)  # revealed: Literal[True]
@@ -38,3 +40,9 @@ reveal_type(n1 is not a1)  # revealed: Literal[True]
 reveal_type(a1 is not o)  # revealed: bool
 reveal_type(n1 is not o)  # revealed: bool
 ```
+
+## Bar
+
+```py
+x
+```

Test Plan

cargo test

@AlexWaygood AlexWaygood added the red-knot Multi-file analysis & type inference label Nov 8, 2024
@AlexWaygood AlexWaygood force-pushed the alex/better-mdtest-output branch from b81bb68 to 3130870 Compare November 8, 2024 22:33
Copy link
Contributor

github-actions bot commented Nov 8, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@AlexWaygood AlexWaygood marked this pull request as draft November 8, 2024 22:49
@AlexWaygood AlexWaygood force-pushed the alex/better-mdtest-output branch 2 times, most recently from f0658db to 079b603 Compare November 8, 2024 23:48
@AlexWaygood AlexWaygood marked this pull request as ready for review November 8, 2024 23:50
@AlexWaygood AlexWaygood force-pushed the alex/better-mdtest-output branch from 079b603 to 5908de8 Compare November 9, 2024 11:06
Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love how long the file path prefix for every error message is now, but if this is what's required to make them clickable in more editors, I guess it's worth it? I don't use the clickability myself. Don't really see a better solution, other than moving the mdtest files to some shorter path relative to workspace root.

It's probably possible to only include the file name in the test name if there is no header? Though it also doesn't bother me to always include it.

On the whole I don't have super strong preferences here so you could wait for a review from Micha, who probably has more opinions. But this looks fine to me.

@AlexWaygood
Copy link
Member Author

I don't love how long the file path prefix for every error message is now, but if this is what's required to make them clickable in more editors, I guess it's worth it? I don't use the clickability myself.

I don't use the clickability inside my IDE much either, but what I do do is run tests from a terminal window outside of my IDE. I'm finding it consistently annoying right now that I can't copy and paste the path directly from the mdtest output so that I can instantly open the Markdown file in question in a new text-editor window. So while I agree that some output lines are now very long with this PR, this would be quite a big quality-of-life improvement for me.

It's probably possible to only include the file name in the test name if there is no header?

I'm sure it is possible. I looked into it quickly, and it seemed more fiddly than I expected, though, and it didn't seem to me to be important enough for me to spend a lot of time on it. This was easy to do, and seemed like an improvement on the status quo.

@sharkdp
Copy link
Contributor

sharkdp commented Nov 11, 2024

I don't love how long the file path prefix for every error message is now, but if this is what's required to make them clickable in more editors, I guess it's worth it? I don't use the clickability myself. Don't really see a better solution, other than moving the mdtest files to some shorter path relative to workspace root.

We could use OSC 8 hyperlinks to potentially have the best of both worlds? We can keep the file path in the output short and provide the full path (including the line number) as a hyperlink.

The problem is that there is no standardized format for "open file F at line L in [your editor of choice]". Some applications solve this by making the hyperlink format customizable. If this doesn't sound too over-engineered, we could maybe provide 2-3 common formats (and a fallback to simple file://hostname/path/to/file links if nothing else was specified) and let every developer configure it (env. variable?!).

Example for VS Code (adapt the /path/to/ruff/ and paste into a terminal to try. vscode://file/ is the prefix that needs to be kept intact):

printf '\033]8;;vscode://file/path/to/ruff/crates/red_knot_python_semantic/resources/mdtest/comparison/identity_tests.md:19\033\\comparison/identity_tests.md:19\033]8;;\033\\\n'

Example with basic file:// link; no support for line numbers; hostname is mandatory.

printf '\033]8;;file://localhost/path/to/ruff/crates/red_knot_python_semantic/resources/mdtest/comparison/identity_tests.md\033\\comparison/identity_tests.md:19\033]8;;\033\\\n'

@MichaReiser
Copy link
Member

On the whole I don't have super strong preferences here so you could wait for a review from Micha, who probably has more opinions. But this looks fine to me.

I've been pretty annoyed by the links not being clickable for the few tests I wrote. That's why I find this a huge improvement.

IMO, the ultimate solution is to use a multiline representation to render the diagnostics, so that the file names no longer stand out that much

Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't get rid of these entirely, because I believe it's possible to have a Markdown test file that doesn't have any # headings in it, which I believe would result in an empty title if we got rid of the path from the title entirely.

Could we only show the filename if the title is empty or change the test framework so that it defaults to the filename if the test has no explicit title?

Reduce the indentation of the line-by-line error reports from 4 spaces to 2. I'm not wild about getting rid of these entirely for the reasons I gave in

This might be a problem for accessibility reasons. I'm still leaning towards removing them entirely but I can also do this when we rework our diagnostics ;)

Thanks

@AlexWaygood
Copy link
Member Author

Could we only show the filename if the title is empty or change the test framework so that it defaults to the filename if the test has no explicit title?

I already answered this in #14213 (comment) ;-) I'm sure it's possible, but didn't want to spend too much time on it

We could use OSC 8 hyperlinks to potentially have the best of both worlds? We can keep the file path in the output short and provide the full path (including the line number) as a hyperlink.

This could be great!! I'll merge this as-is for now, though, as I think we're aligned that, even though the output still isn't ideal, this is probably a strict improvement on the status quo :-) This feels like it could be pursued as a followup

@AlexWaygood AlexWaygood merged commit 813ec23 into main Nov 11, 2024
20 checks passed
@AlexWaygood AlexWaygood deleted the alex/better-mdtest-output branch November 11, 2024 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants