[ty] Add hyperlinks to rule codes in CLI#21502
Conversation
MichaReiser
added
ty
| use std::ops::Range; | ||
|
|
||
| #[derive(Copy, Clone, Debug, Default, PartialEq)] | ||
| pub(crate) struct Id<'a> { |
There was a problem hiding this comment.
This type is inspired by the new annotate snippet that also has an Id type
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
MichaReiser
requested review from
AlexWaygood,
BurntSushi,
carljm,
dcreager and
sharkdp
as code owners
|
MichaReiser
changed the title
|
Interesting that mypy_primer timed out but ecosystem-analyzer didn't find any changes to report (primer timeouts usually occur because it took too long trying to compute the diff between the two runs) |
|
mypy primer timing out makes sense, given that it forces color output (every diagnostic has now a different output) |
MichaReiser
force-pushed
the
micha/error-links
branch
from
c13f7b6 to
3777918
Compare
|
There's an argument that it's hard to notice that the id is clickable. We could decide to underline the id or add the URL as a note (similar to what we do with the rule status). This would have the benefit that we could render the URL as a string and it's up to the CLI to add the hyperlink. The downside of this is that it only works for the I'm also not a 100% sure if we need to restrict the hyperlink rendering to more cases, e.g. by using https://docs.rs/supports-hyperlinks/latest/src/supports_hyperlinks/lib.rs.html#12-53 |
BurntSushi
left a comment
BurntSushi
left a comment
There was a problem hiding this comment.
Love it! Nice work. This all LGTM. I agree that the overhead here is likely negligible, and so preferring API simplicity is the right call.
There's an argument that it's hard to notice that the id is clickable. We could decide to underline the id or add the URL as a note (similar to what we do with the rule status). This would have the benefit that we could render the URL as a string and it's up to the CLI to add the hyperlink. The downside of this is that it only works for the full output and not concise.
I like the underline.
I'm also not a 100% sure if we need to restrict the hyperlink rendering to more cases, e.g. by using https://docs.rs/supports-hyperlinks/latest/src/supports_hyperlinks/lib.rs.html#12-53
I'd be fine with doing this reactively, i.e., in response to user feedback.
We could also choose to add a note containing the actual URL if and only if we decide not to render the hyperlink (based on supports-hyperlink).
But I'm fine shipping this as-is and iterating. :-)
|
CMD+clicking on the rule code works on this branch for me in a terminal inside VSCode, but not in the Terminal app on my macbook -- any idea why? (Is this just a case of "Alex, start using iTerm2 or something instead of the default Terminal app that your macbook came with"?) Adding the full URL as a note might resolve this, since I'm definitely able to CMD+click on full URL in a terminal (I do this all the time to open local playground deploys in a browser straight from my terminal, obviously 😄) |
I couldn't find a reference but yes, this is the behavior of terminals supporting the ANSI escape but not supporting the hyperlink feature. |
|
I actually like the current behavior where Ghostty only renders the underline when I hit Cmd and hover the id. This makes it behave the same as other clickable links |
Summary -- This PR wires up the `Diagnostic::set_documentation_url` method from #21502 to Ruff's lint diagnostics and renders them in the grouped output format (concise and full are already covered by #21502). I considered also including the URLs for the pylint output format, but it doesn't currently render any color in the CLI, so it may be a machine-readable format too? Test Plan --
Summary -- This PR wires up the `Diagnostic::set_documentation_url` method from #21502 to Ruff's lint diagnostics. This enables the links for the full and concise output formats without any other changes. I considered also including the URLs for the grouped and pylint output formats, but the grouped format is still in `ruff_linter` instead of `ruff_db`, so we'd have to export some additional functionality to wire it up with `fmt_with_hyperlink`; and the pylint format doesn't currently render with color, so I think it might actually be machine readable rather than human readable? The other ouput formats (json, json-lines, junit, github, gitlab, rdjson, azure, sarif) seem more clearly not to need the links. Test Plan -- I guess you can't see my cursor or the browser opening, but it works for lint rules, which have links, and doesn't include a link for syntax errors, which don't have valid links. 
3 participants
Summary
This PR adds hyperlink rendering to our diagnostics so that users can jump to the rule documentation by clicking on the diagnostic id.
My first implementation added an optional
url_resolver: &dyn Fn(&DiagnosticId) -> Option<String>argument to the full and concise rendering. The main idea was to avoid constructing the URLs unnecessarily when the output format doesn't need the URL.However, I then realised that the LSP always uses the URL and we have other output formats (ruff only for now) that construct the URL too, namely the JSON and rdjson output formats. The overhead also seems negligible compared to that we always construct the full diagnostic, even when only using the
conciseoutput format. That's why I opted to store the URL as an optional field on the diagnostic instead. Not only did it simplify the implementation, it also reduced some code duplication.The main advantage of this approach is that it requires some amount of code duplication to set the URL on the diagnostic.
It should be very easy to leverage this functionality in Ruff too (@ntBre if you're interested).
Test Plan
Screen.Recording.2025-11-18.at.09.14.16.mov