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

Proposal: Add URL for a rule's documentation to the API #6582

Closed
pmcelhaney opened this issue Jul 2, 2016 · 49 comments
Closed

Proposal: Add URL for a rule's documentation to the API #6582

pmcelhaney opened this issue Jul 2, 2016 · 49 comments
Assignees
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint

Comments

@pmcelhaney
Copy link
Contributor

pmcelhaney commented Jul 2, 2016

The version of ESLint you are using.

4.12.1

The problem you want to solve

AtomLinter has a neat feature that links to the documentation for each rule.

Atom screenshot showing link

In order for that link to work in plugins, the plugin needs to be registered in AtomLinter.

Your take on the correct solution to problem

Instead of AtomLinter having to maintain a list of known plugins (and other clients having to duplicate the effort) it would be great if the API could allow a rule to specify where its documentation can be found.

It boils down to:

  1. Exposing a rule's meta object via the API (which is a one-liner though several test fixtures are affected).
  2. Establishing an optional convention of putting the URL describing a rule in docs.uri / docs.url.
  3. Adding a URL to the meta for each of the built-in rules.

Original proposal which worked at the plugin level rather than the rule level.

module.exports = {
    rules: {
        ...
    },
    ...

    // New property
    getDocURI: function (ruleName) { 
       return `https://myplugin.com/docs/${ruleName}.md`
    }
};

The URL would be exposed to the Node API via the response from the verify() method.

{
    fatal: false,
    ruleId: "semi",
    severity: 2,
    line: 1,
    column: 23,
    message: "Expected a semicolon.",
    fix: {
        range: [1, 15],
        text: ";"
    },
    docURI:  "http://eslint.org/docs/rules/semi" // <-- New property
}

I've implemented the change in ESLint. I'm waiting for the proposal to be accepted so I can open a PR. If it's accepted I'm happy to do the corresponding update for AtomLinter as well.

@eslintbot eslintbot added the triage An ESLint team member will look at this issue soon label Jul 2, 2016
@platinumazure
Copy link
Member

platinumazure commented Jul 2, 2016

It'd be good to discuss the details a little. For example, first question I have is why not have the URLs collected in a plugin definition file (same as where plugin rules and configurations are exported)? This also allows the same file to contain a URL for overall documentation, if needed. Proposal for that API is here: #6525

@ilyavolodin
Copy link
Member

This can probably be done very easily through rule's metadata. No code changes necessary. Although we do need an API to retrieve metadata.

@ilyavolodin ilyavolodin added enhancement This change enhances an existing feature of ESLint core Relates to ESLint's core APIs and features evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Jul 2, 2016
@pmcelhaney
Copy link
Contributor Author

pmcelhaney commented Jul 3, 2016

Kevin, what you describe is what I have in mind.

I thought about putting the URL in [rule] metadata, but that would become very redundant. Rather define one function at the plugin level.

The concept is more or less proven by AtomLinter. We'd just be moving the knowledge of URIs from AtomLinter to the plugin itself, and expose it through an API that AtomLinter and any other client can consume.

On Jul 2, 2016, at 7:03 PM, Kevin Partington notifications@github.com wrote:

It'd be good to discuss the details a little. For example, first question I have is why not have the URLs collected in a plugin definition file (same as where plugin rules and configurations are exported)? This also allows the same file to contain a URL for overall documentation, if needed.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

@pmcelhaney
Copy link
Contributor Author

I've updated the issue to clarify exactly what API additions I'm proposing. This would be a new feature without any breaking changes.

@vitorbal
Copy link
Member

vitorbal commented Jul 3, 2016

Why would you say putting the URL in the metadata property would be redundant? This seems like the perfect use case for a metadata property.

@platinumazure
Copy link
Member

I guess I'm not attached one way or the other. I agree it would be great to use some sort of metadata property (either at plugin or rule level) to provide documentation links.

@ilyavolodin Perhaps you want to take a look at the first post of this issue, since it has been updated with a concrete proposal?

@pmcelhaney
Copy link
Contributor Author

I'm in favor of configuring the URI at the plugin level because it saves plugin authors from having to include the URI in every single rule.

@pmcelhaney
Copy link
Contributor Author

First stab at a PR. Feedback much appreciated.

#6589

pmcelhaney added a commit to pmcelhaney/eslint that referenced this issue Jul 4, 2016
pmcelhaney added a commit to pmcelhaney/eslint that referenced this issue Jul 4, 2016
@mysticatea
Copy link
Member

mysticatea commented Jul 4, 2016

I vote to a use of meta property.

@ilyavolodin
Copy link
Member

I understand redundancy argument. However, I feel like "one URL to rule them all" is too inflexible. For example, there might be plugin authors, that don't want to name all of their pages with the same name as rule name. Or they might use some provider that doesn't, for example, support dashes in the urls, but their rules have dashes.
I do understand that you can write complicated implementation of ruleURI to support the above listed edge cases, but it feels like recreating the purpose that metadata is supposed to cover.

One other thing to keep in mind, most of the editor addons that might/would use this functionality don't know anything about plugins. So when the error occurs, they would need to parse rule name, extract plugin name out of it, then look up rule URI for it in the API. Vs. just calling API and comparing rule name against the list. No parsing required.

And last thing is, this proposed solution would not work for custom rules, just for plugins. Metadata would work for both.

@pmcelhaney
Copy link
Contributor Author

Thanks, @ilyavolodin. Good thoughts.

I don't think editors (and their addons) have to deal wirth ESLint plugins. It's all transparent. A consumer of the Node API shouldn't need to do any parsing. It gets the URI for a reported problem and can do what it wants with it. Anyway, how the URI is exposed in the Node API is orthaganol to whether the URI is configured by a plugin-level function or rule-level metadata.

AtomLinter has already proven the concept of using a function to map a ruleId to a URI. All of the functions that have been registered so far have been trivial one-liners.

Your point about not working for custom rules is valid. However I don't imagine there will be a huge demand for this feature to be used in one-off custom rules. How many people go to the trouble of publishing docs for project-specific rules? And if they do, what's stopping them from moving the rules to a plugin that can be shared across multiple projects?

Also there's an advantage to generating a URI on the fly. In the future, the getDocURI() function could take the problem object as an optional second parameter. That would allow a plugin to generate a URI that points to "smart" documentation, which is tailored to use your actual code snippet in the explanation.

@pmcelhaney
Copy link
Contributor Author

  • @granteagon @relekang @IanVS Looks like you were involved in adding the ESLint plugin docs to the AtomLinter plugin (thank you!). I'm proposing that we allow ESLint plugins to specify where to find their docs and thought you might have something to add to the discussion.

@granteagon
Copy link

@pmcelhaney, do you mean you are proposing "AtomLinter plugins to specify where to find their docs"? I did help get the ESLint docs working.

Being out of the loop on the conversation a bit, I think that @ilyavolodin is tending towards a solution I would support. A blanket "base URL" is probably not a flexible solution. Meta-data is probably better.

In my opinion, it would be easier to create a build script on the plugins to generate the full URLs for each rule, than it would be to rely on each plugin author to follow a more complex pattern. Just my two cents.

Great idea. I've been thinking this should be done for some time now. I'm happy to help in anyway I can. Keep me in the loop. I'm going to loop in @Arcanemagus who helped me implement my code. He's a pretty smart dude who may be able to help us come to a conclusion.

@nzakas
Copy link
Member

nzakas commented Jul 4, 2016

I agree with @ilyavolodin that this information really belongs in meta, probably meta.docs.

On a larger point: all of ESLint is meant so that rules are rules are rules -- we try very hard not to treat rules from the core different from custom rules or different from plugin rules. Any solution we choose must keep that design principle in mind even if it leads to some redundancies.

@pmcelhaney
Copy link
Contributor Author

Okay, thanks. I'll send another PR that puts the config in meta.

@nzakas
Copy link
Member

nzakas commented Jul 4, 2016

@pmcelhaney it would be helpful (and arguably a better use of your time) to propose a design here for review before submitting a new PR.

pmcelhaney added a commit to pmcelhaney/eslint that referenced this issue Jul 4, 2016
pmcelhaney added a commit to pmcelhaney/eslint that referenced this issue Jul 4, 2016
@pmcelhaney
Copy link
Contributor Author

Thanks @nzakas. Unfortunately by the time I saw your comment I already submitted a new PR. As yourself and others have suggested, I simply exposed the rule metadata through the API (as part of the problem object) and documented meta.docs.uri as the standard place to put the URL for a rule's documentation.

(Aside: I know you've expressed frustration about people reporting bugs / requesting features and not being respectful of the fact that you're all volunteers. Submitting PRs is my way of showing that I'm here to contribute, not just to pester you and expect something for nothing.)

@nzakas
Copy link
Member

nzakas commented Jul 5, 2016

@pmcelhaney and we appreciate the PRs, we're just trying to be respectful of your time by encouraging you to describe what you're thinking of being digging into the code. As this is a change to the core, we have to be more careful about evaluating and accepting it.

@pmcelhaney
Copy link
Contributor Author

pmcelhaney commented Jul 5, 2016

From my point of view, the clearest and most efficient way to express my thinking is to just send a PR. Not necessarily a complete PR, but one that hits the key tests, doc updates, and code changes. In this case, the change was relatively small so it was just as easy to do the whole thing (sans adding meta.doc.uri to all of the built-in rules).

Anyway, if outlining the proposed design in prose works better for you, I'll keep that in mind going forward.

On Jul 5, 2016, at 3:36 PM, Nicholas C. Zakas notifications@github.com wrote:

@pmcelhaney and we appreciate the PRs, we're just trying to be respectful of your time by encouraging you to describe what you're thinking of being digging into the code. As this is a change to the core, we have to be more careful about evaluating and accepting it.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@Arcanemagus
Copy link

Additionally, we decided to add a CLIEngine#getRules method to allow integrations that use CLIEngine to access the rules (as opposed to integrations that only use Linter).

Yay! No more undocumented hack needed!

@pmcelhaney pmcelhaney changed the title Propsal: Add URL for a rule's documentation to the API Proposal: Add URL for a rule's documentation to the API Dec 27, 2017
pmcelhaney added a commit to pmcelhaney/eslint that referenced this issue Dec 27, 2017
pmcelhaney added a commit to pmcelhaney/eslint that referenced this issue Dec 28, 2017
pmcelhaney added a commit to pmcelhaney/eslint that referenced this issue Dec 28, 2017
pmcelhaney added a commit to pmcelhaney/eslint that referenced this issue Dec 28, 2017
pmcelhaney added a commit to pmcelhaney/eslint that referenced this issue Dec 28, 2017
pmcelhaney added a commit to pmcelhaney/eslint that referenced this issue Dec 28, 2017
Code mod:

export default function transformer(file, api) {
    const j = api.jscodeshift;

    const name = file.path.split('/').pop().replace(/.js$/, '');

    return j(file.source)
      .find(j.ObjectExpression)
      .filter(path => path.parentPath.node.key && path.parentPath.node.key.name === 'docs')
      .forEach(path => {
        const prop = j.property('init', j.identifier('url'), j.literal(`https://eslint.org/docs/rules/${name}`));
        j(path).replaceWith(j.objectExpression([...path.node.properties, prop]));
      })
      .toSource({lineTerminator: '\n'});
  }
pmcelhaney added a commit to pmcelhaney/eslint that referenced this issue Dec 28, 2017
Code mod:

export default function transformer(file, api) {
    const j = api.jscodeshift;

    const name = file.path.split('/').pop().replace(/.js$/, '');

    return j(file.source)
      .find(j.ObjectExpression)
      .filter(path => path.parentPath.node.key && path.parentPath.node.key.name === 'docs')
      .forEach(path => {
        const prop = j.property('init', j.identifier('url'), j.literal(`https://eslint.org/docs/rules/${name}`));
        j(path).replaceWith(j.objectExpression([...path.node.properties, prop]));
      })
      .toSource({lineTerminator: '\n'});
  }
pmcelhaney pushed a commit to pmcelhaney/eslint that referenced this issue Dec 29, 2017
Add a lint rule to ensure that each rule has a meta.docs.url property with the correct value.
pmcelhaney pushed a commit to pmcelhaney/eslint that referenced this issue Dec 29, 2017
Add a lint rule to ensure that each rule has a meta.docs.url property with the correct value.
pmcelhaney added a commit to pmcelhaney/eslint that referenced this issue Dec 31, 2017
ilyavolodin pushed a commit that referenced this issue Jan 5, 2018
* Docs: add url to each of the rules (refs #6582)

Code mod:

export default function transformer(file, api) {
    const j = api.jscodeshift;

    const name = file.path.split('/').pop().replace(/.js$/, '');

    return j(file.source)
      .find(j.ObjectExpression)
      .filter(path => path.parentPath.node.key && path.parentPath.node.key.name === 'docs')
      .forEach(path => {
        const prop = j.property('init', j.identifier('url'), j.literal(`https://eslint.org/docs/rules/${name}`));
        j(path).replaceWith(j.objectExpression([...path.node.properties, prop]));
      })
      .toSource({lineTerminator: '\n'});
  }

* New: internal-rules/consistent-docs-url (refs #6582)

Add a lint rule to ensure that each rule has a meta.docs.url property with the correct value.
@not-an-aardvark
Copy link
Member

I think this issue can be closed now that we've added CLIEngine#getRules and established the meta.docs.url convention for rules.

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Mar 11, 2019
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Mar 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint
Projects
None yet
Development

No branches or pull requests