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

Add link to ESLint rules via url attribute of the Diagnostic API #562

Closed

Conversation

thibaudcolas
Copy link

@thibaudcolas thibaudcolas commented Oct 21, 2018

Fixes #151. Relies on microsoft/vscode#61436 to implement diagnostic URLs in VS Code (fixing microsoft/vscode#11847), as well as microsoft/vscode-languageserver-node#432.

This PR does three things:

  1. Add eslint-rule-documentation to retrieve URLs as a fallback to the rules metadata, so older versions of ESLint / older versions of plugin rules can also benefit from URLs.
  2. Make the URL retrieval work with versions of ESLint before v4.15.0 (2018-01-06), which introduced CLIEngine#getRules().
  3. Use the (new) url attribute of diagnostics to forward the ESLint rule's URL to VS Code, for display wherever.

Changes 1. and 2. could be useful now without further changes in VS Code or anywhere else. I can make separate PRs for these if they seem desirable enough.

Here is what the end result looks like at the moment (WIP):
wip-highlight-over-link


The instructions to try this out locally are at the end of microsoft/vscode#61436.

@msftclas
Copy link

msftclas commented Oct 21, 2018

CLA assistant check
All CLA requirements met.

* Licensed under the MIT License. See License.txt in the project root for license information.
* ------------------------------------------------------------------------------------------ */
declare module "eslint-rule-documentation" {
export default function ruleURI(ruleId: string): { found: boolean, url: string };
Copy link
Author

Choose a reason for hiding this comment

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

This isn't the correct type signature – the function is a CommonJS module.exports. I couldn't figure out how to type those correctly.

Copy link
Member

Choose a reason for hiding this comment

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

A good doc on how to create these declaration files can be found here: https://www.typescriptlang.org/docs/handbook/declaration-files/introduction.html

Copy link
Author

Choose a reason for hiding this comment

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

Arr thank you, I had looked at these docs but totally missed the relevant template (https://www.typescriptlang.org/docs/handbook/declaration-files/templates/module-function-d-ts.html). Will update this later.

ruleURLs.set(problem.ruleId, url);
}
// @ts-ignore TODO Ignoring "property URL does not exist" for now. It will exist in the newest version of vscode.
diagnostic.url = url;
Copy link
Author

Choose a reason for hiding this comment

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

This type error will go away when relying on a version of VS Code that has a URL attribute for diagnostics. For local devs, I was using an npm linked version, but that didn't seem to be enough.

@@ -819,6 +834,26 @@ function validate(document: TextDocument, settings: TextDocumentSettings, publis
}

let cli = new settings.library.CLIEngine(newOptions);

// cache documentation urls for all rules
if (cli.getRules) {
Copy link
Author

Choose a reason for hiding this comment

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

As far as I can tell, the implementation in master would fail for ESLint versions below v4.15.0 (jan 2018).

if (!url) {
// @ts-ignore TODO: Should type-check the 3rd-party module.
url = ruleURI(problem.ruleId).url;
ruleURLs.set(problem.ruleId, url);
Copy link
Author

Choose a reason for hiding this comment

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

If the URL isn't in the cache, it's not from the rule's metadata. So we use eslint-rule-documentation directly, and then set its result in the cache.

@thibaudcolas
Copy link
Author

Closing because of microsoft/vscode#61436 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a link to ES Lint rule page.
3 participants