-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Only render hyperlinks for terminals known to support them #21519
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
Conversation
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
| 4 | | ||
| note: This is an unsafe fix and may change runtime behavior | ||
| "); | ||
| insta::assert_snapshot!(env.render_diagnostics(&diagnostics)); |
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 made those tests "normal" snapshot tests because some snapshot lines contain trailing whitespace that Zed strips (might be a bug in Zed as I don't think an editor should strip trailing whitespace in a string!).
I spent some time yesterday trying to fix the trailing whitespace but without success.
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.
Yeah I support this. My editor (nvim) is configured to do the same.
|
sharkdp
left a comment
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.
That library looks reasonable. And I don't mind if this buys us a few "false negatives".
BurntSushi
left a comment
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.
LGTM
| 4 | | ||
| note: This is an unsafe fix and may change runtime behavior | ||
| "); | ||
| insta::assert_snapshot!(env.render_diagnostics(&diagnostics)); |
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.
Yeah I support this. My editor (nvim) is configured to do the same.
* origin/main: [ty] Fix flaky tests on macos (#21524) [ty] Add tests for generic implicit type aliases (#21522) [ty] Semantic tokens: consistently add the `DEFINITION` modifier (#21521) Only render hyperlinks for terminals known to support them (#21519) [ty] Keep colorizing `mypy_primer` output (#21515) [ty] Exit with `2` if there's any IO error (#21508) [`ruff`] Fix false positive for complex conversion specifiers in `logging-eager-conversion` (`RUF065`) (#21464) [ty] tighten up handling of subscripts in type expressions (#21503)
* origin/main: [ty] Fix flaky tests on macos (#21524) [ty] Add tests for generic implicit type aliases (#21522) [ty] Semantic tokens: consistently add the `DEFINITION` modifier (#21521) Only render hyperlinks for terminals known to support them (#21519) [ty] Keep colorizing `mypy_primer` output (#21515) [ty] Exit with `2` if there's any IO error (#21508) [`ruff`] Fix false positive for complex conversion specifiers in `logging-eager-conversion` (`RUF065`) (#21464) [ty] tighten up handling of subscripts in type expressions (#21503)
Summary
GitHub action's renderer doesn't support hyperlink rendering and, unlike the terminals I tested,
it also isn't just ignoring the hyperlink escape sequence, instead it removes/hides the entire content between the Hyperlink escape sequence.
This PR uses
supports-hyperlinksto only render hyperlinkson terminals that are known to support the escape sequence.
The crate is used by cargo and miette.
Test Plan
Is mypy primer timing out proof enough that ty no longer renders the hyperlink ANSI escapes in CI 😅?
I tested that the hyperlinks are clickable in Ghostty