Ability to add links to diagnostics and render them as links in problems view
darkred opened this issue · 47 comments
For example, linting var test = 1
results in these warnings in the Problems panel:
My suggestion:
the no-unused-vars and semi to be clickable and to link to
http://eslint.org/docs/rules/no-unused-vars and to
http://eslint.org/docs/rules/semi
respectively.
(I've found this feature in AtomLinter and it's really useful)
cc: @dbaeumer
PS. Using vscode-eslint 1.10.18 with ESLint 3.5.0 in VSCode 1.5.1 using win10 x64.
Like the idea of having a link in the problems panel to point to documentation. @sandy081
@dbaeumer Does the problem diganostics have information about these external links?
I would like to take a stab at this if no work has already been done. FYI looks the Atom linter-eslint folks pulled this logic out into a standalone module we could use pretty easily: https://github.com/jfmengels/eslint-rule-documentation
@bpugh That would be great. No work has not been started. Let me know if you need any assistance.
Thanks! I did have one question though. It looks pretty straightforward to add the url to the message from vscode-eslint but we're currently using the diagnostics api to display the messages in the problems panel but it doesn't seem to allow anyway to link to an external url. Would this require a PR to vscode as well?
+1
@sandy081 I believe @bpugh was asking you that question but didn't tag you.
p.s. @rmartins90 @kuschti You can thumbs up a comment directly instead of adding spam.
@bpugh Yes, this needs to be implemented in VS Code too, to detect and render links in Problems view.
No progress on this one?
Would be totally handy!
I'd be a fan, too, including linking to optional external URLs... for example, I have an extension that integrates with known vulnerabilities for certain components; I'd love to be able to link in the problem space to the actual advisory URL with more details... today folks have to copy/paste from the Diagnostic message/text to get the URL out.
+1
Hey guys, how are things going
I would like to work on this if no one else is. I stumbled upon this as part of Microsoft's Hacktoberfest participation! This is my first time working in the vscode codebase so I spent a bit of time looking into how to achieve this. Here is my progress so far.
Reference implementation – Atom and its linter-eslint
- Linter Message API has a
urlattribute used for this - Generates the message URL from the ESLint result: https://github.com/AtomLinter/linter-eslint/blob/3630ab63e853bb98afd99e142143642b1569708b/src/helpers.js#L296-L298
Uses ESLint's rules metadata, or eslint-rule-documentation if no metadata is available https://github.com/AtomLinter/linter-eslint/blob/4b7334005b58f4dfbcdd239a2eed6e516d8ffe4f/src/rules.js#L39-L56
The rules metadata is only exposed starting with ESLint v4. Ideally would be retrieved with CLIEngine#getRules(), introduced in v4.15.0. So eslint-rule-documentation is a fallback for older versions, and third-party plugins that do not expose rules metadata (yet).
See https://github.com/AtomLinter/linter-eslint/blob/3630ab63e853bb98afd99e142143642b1569708b/src/worker-helpers.js#L189-L211 for the rules retrieval from ESLint.
This is what it looks like in the end:
VS Code implementation
vscode part
Introduce a new url attribute on the Diagnostic API, then different parts of the UI can choose whether to use this URL to render a link, or not.
- Some parts of the UI already support rendering links via Markdown, so it shouldn't be too much work.
- I'm still struggling to figure out which widget to do this inside of
😅 .
vscode-eslint part (or any other extension)
Use the new url attribute of the Diagnostic, populating it based on the metadata from CLIEngine#getRules() if available, or eslint-rule-documentation otherwise.
Add an optional getRules here: https://github.com/Microsoft/vscode-eslint/blob/ddc2dff0b5d01045b635cd55975a1e01fef8d732/server/src/eslintServer.ts#L147-L149
Load (and cache?) the rules from ESLint (with getRules) somewhere – I'm not too familiar with the extensions lifecycle just yet.
Set the URL in makeDiagnostic: https://github.com/Microsoft/vscode-eslint/blob/ddc2dff0b5d01045b635cd55975a1e01fef8d732/server/src/eslintServer.ts#L160-L178
Next step is for me to figure out where exactly the Diagnostics are displayed, and then render a Markdown link there if the url is present. Perhaps also add a "code action" available via the contextual menu to open this URL in the browser.
Progress update – I think I figured out all of the places that would/could support those links:
- The hover modal, where at the moment content is displayed as a code block (I added the link shown in the screenshot)
- The F8 panel
- The Problems tree viewer, with its highlighted labels for filtering
If anyone has thoughts on how to handle the URLs in each of these, please let me know.
Attempted a PR (well, 3 PRs) for this over at:
Feedback welcome, especially on the UI side! If anyone wants to try it out locally, the instructions are at the end of #61436.
Nice work. I am willing to cover the LSP & eslint work if we decide to support this. One thing I noticed by quickly looking at the change. I would name the property docUrl to make it clear that this refers to a documentation and not to a resource.
Stupid question, but why isn't DiagnosticRelatedInformation sufficient for this? The url property looks a little like some bolt on API to me and nothing that I would naturally expect in this domain.
Also, if the sole purpose is to render a link with a diagnostic message then we should consider
#54272 which is about allowing diagnostic messages to be markdown formatted text.
@jrieken definitely not a stupid question – you probably understand those APIs much more than I ever will. My understanding is that DiagnosticRelatedInformation is meant to point to code locations. It feels strange to use this for a URL that's always going to point at the same location as the diagnostic.
I'll comment further in #61436, as most of the feedback so far is about the implementation.
My PR(s) got closed pending further discussion, so I won't work on this further.
If we have to use the message to include the linked code, then the code is going to be displayed twice (once in the message and once separately) – not sure if that's a good idea.
I tend to agree with @thibaudcolas – it makes sense to associate the code with a URL.
After going through all comments, it seems there is a need to link error code to a location that has information about the error code. Since this is very specific to error code, it does not makes sense to add URI property to diagnostic. Instead the code property should contain the link. Example:
code?: string | number | { code: string | number, link: Uri };
@jrieken What do you think?
I think that's overloading the code property too much. Why don't we use DiagnosticRelatedInformation?
That would for ESLint add a child for every diagnostic object. Not sure if that will look nice.
Why don't we use DiagnosticRelatedInformation?
Not sure if it's a good idea to use a property for an unrelated purpose. I think code property is the right one to have information about link that explains code.
From @DanTup
using RelatedInformation sounds wonky to me, and I don't think supporting formatted content in errors solves the problem (I don't want to clutter the error list even more with visible links, the context menu is an idea place for it).
Since no one agrees with using relatedInformation, are there any further objections to @sandy081's suggestion? It seems like the most logical option.
@sandy081 has there been any more discussion on this? It would be really nice for users to get these links in somewhere (and I think your proposal of joining them with code makes a lot of sense).
code?: string | number | { code: string | number, link: Uri };
Thinking about this again - for LSP I think this would be breaking and need an opt-in flag/capability (existing clients would choke on the new format). If url was added as its own optional property, that would be simpler?
No discussions.
@sandy081 I don't suppose there's been any progress on this? I think this would be a neat feature for all languages and there's no easy workaround (if we inject our own code actions for this we have to mark them as quick-fixes, which will cause them to show up in places that don't make sense - like if the user has a quick-fix keybind).
Seems like a trivial thing to implement and would make the user experience soo much better. Please add
Doesn't the new MDN reference link that was recently added to the hover message make this a little easier?
MDN Reference for HTML and CSS
VS Code now displays a URL pointing to the relevant MDN Reference in completion and hover of HTML & CSS entities
@dbaeumer @jrieken Please let me know your opinion about adding links to code property?
I already did: #11847 (comment)
If the link is added to related information, how would it be rendered? I'd really like to have a context-menu action to launch help documentation for an error, but I really do not want every message to become expanded with a visible hyperlink in the problems view - that would be incredibly spammy (especially when many of them are the same links).
Proposed API
export interface Diagnostic {
/**
* Will be merged into `Diagnostic#code`
*/
code2?: {
/**
* A code or identifier for this diagnostic.
* Should be used for later processing, e.g. when providing [code actions](#CodeActionContext).
*/
value: string | number;
/**
* A link to a URI with more information about the diagnostic error.
*/
link: Uri;
}
}Sample usage
const fooErrors = vscode.languages.createDiagnosticCollection('foo');
let disposable = vscode.commands.registerCommand('extension.helloWorld', () => {
fooErrors.clear();
const activeDocUri = vscode.window.activeTextEditor!.document.uri;
fooErrors.set(activeDocUri, [
{
range: new vscode.Range(0, 0, 0, 5),
message: 'You should always use semicolon;',
source: 'eslint',
code: '@typescript-eslint/semi',
code2: {
value: '@typescript-eslint/semi',
link: vscode.Uri.parse('https://eslint.org/docs/rules/semi')
},
severity: vscode.DiagnosticSeverity.Error,
},
{
range: new vscode.Range(1, 0, 1, 5),
// message: 'Ensures that double quoted strings are passed to a localize call\nto provide proper strings for\ndifferent locales',
message: 'Ensures that double\nquoted strings are passed to a localize call to provide proper strings for different locales',
source: 'eslint',
code2: {
value: 'code-no-unexternalized-strings',
link: vscode.Uri.parse('https://github.com/microsoft/tslint-microsoft-contrib/blob/master/src/noUnexternalizedStringsRule.ts')
},
severity: vscode.DiagnosticSeverity.Error,
}
]);
});Behavior
- The link is rendered in Problems Panel, Diagnostics Hover and Goto Error View.
- The click behavior is similar to other link clinks: https://code.visualstudio.com/docs/editor/codebasics#_multicursor-modifier
- The tooltip updates as you change
editor.multiCursorModifier
Moving this to finalisation phase as this is a simple addition.
Moving this to finalisation phase as this is a simple addition.
There is no API sync this week, we will not finalise this during January. Please keep the item open and schedule the finalisation for Feb
wrt to the API proposal, we should use target (or uri) instead of link. The former would align this with the DocumentLink-api.
In SonarLint we are already allowing users to open rule description (using a code action), so this feature would be useful as a replacement. But we are displaying rule description directly inside VSCode (using a web view panel). Do you know if this feature would support URI with custom scheme, so that we could use our own handler and not open an external browser?
Yeah, using whatever uri-scheme should be supported by this but I do wonder why SonarLint uses a web view and not just a browser? Is that your help text is dynamic or private?
Our help text is indeed dynamically generated from metadata coming from the embedded analyzer (rule name, severity, markdown description converted to HTML, ...). This is better for us to rely on embedded analyzer metadata than trying to host online content, since it will perfectly match the analyzer version, and also this will work when user has no internet connexion.
Ok, that makes sense. For us it is important that the link has a direct connection to the error-code, hence the shape of the API and that it isn't a generic link
Yes, this will be added to the LSP as well.
Steps:
- Read this API:
Lines 4676 to 4691 in bddc5ef
- Write an extension that uses this API. Make sure if you provide
target, a link is rendered with thevalueas text and upon clicking opens the link in problems-view/inline-error-view/error-hover. Here's how it works roughly: #11847 (comment)