Docs: add url to each of the rules (refs #6582)

[pmcelhaney]

Automatically generated with this 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'});
  }

What is the purpose of this pull request? (put an "X" next to item)

[X] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)
Added a meta.docs.url property to each of the built in rules.

Is there anything you'd like reviewers to focus on?
I had to manually update no-extra-parens and no-unused-vars because jscodeshift made unwanted changes to those files.

If you want to critique my script, I'm open to that too. I don't have much experience with codemods. It might not be a bad idea to clean up the script and publish it so that it can be used by plugin authors.

[ilyavolodin]

Thanks for doing this. I wonder if we should preserve jscodeshift code in https://github.com/eslint/eslint-transforms so that plugin authors could take advantage of that?

[not-an-aardvark]

Personally, I think it would be better to have the property added dynamically to the rules objects for core rules so that we don't have to duplicate it in every file. This would avoid any issues where e.g. someone forgets to add the property to a new rule.

[platinumazure]

@not-an-aardvark

Personally, I think it would be better to have the property added dynamically to the rules objects for core rules so that we don't have to duplicate it in every file. This would avoid any issues where e.g. someone forgets to add the property to a new rule.

You're not wrong, but we could also add an internal linting rule for that purpose.

[not-an-aardvark]

Sure, a linting rule is a good way to prevent bugs in those lines of code, but a better way to prevent bugs like that is to not have those lines of code at all. It's easy to generate the URL for any given rule at runtime, so duplicating the URL everywhere doesn't seem ideal.

[j-f1]

From #9782 (comment), related to the above discussion:

Would it be possible to add a method on the exported plugin object that generates the docs URL so that people can optionally DRY out their code? (i.e. getDocsURL: name => `https://github.com/foo/eslint-plugin-bar/blob/master/docs/rules/${name}.md` instead of providing meta.docs.url on each rule)

[pmcelhaney]

I wrote an internal linting rule last night. Should I add it to this PR or create a separate one? I've added it to this PR as a separate commit.

[pmcelhaney]

My original thought was to create a method at the plugin level, but it went against the philosophy that each rule is independent.

I’ve come around to that line of thinking. There’s a certain amount of boilerplate that we can never completely DRY out.

There’s a one time retrofit which the codemod makes almost effortless. From then, the only time one needs to think about the URL is right after writing the docs, which is several orders of magnitude more effort.

[not-an-aardvark]

There’s a certain amount of boilerplate that we can never completely DRY out.

As far as core rules are concerned, it seems like this would be a fairly simple refactoring. I agree that exposing it as a convenience for plugins would be more complicated and maybe not worthwhile. (When writing a plugin, I would probably dynamically generate the URLs for the exported rule objects.)

I'm fine with this being merged as-is, but I think switching to dynamically generate the URLs for core rules would be a good change if someone has time for it in the future.

[platinumazure]

@not-an-aardvark @pmcelhaney Would either of you be willing to take a crack at a dynamic approach for adding the URLs automatically based on the rule name? Or at least a quick explanation of what that might look like... I kind of want to evaluate both approaches, and don't feel like I have enough data for the dynamic approach to evaluate. But please don't spend a lot of time on this.

[pmcelhaney]

Some points to consider:

• Adding a new core rule is a rare event these days.
• The URLs have already been automatically (statically) added
• This PR has a rule to (statically) make sure the URL is included and correct
• We can always change our minds and switch to a dynamic method later

I think adding the URLs to core rules as they’re loaded by CLIEngine would be trivial: a forEach and an assignment. I just don’t like the idea that it gives core rules special treatment.

[pmcelhaney]

Having said that, my feelings won’t be hurt if this PR is declined. I’m fine with whatever the team decides and willing to help with the implementation if needed.

[ilyavolodin]

I think it doesn't hurt to merge this in, and if necessary modify it later. Since the work is already done here, it's not a large effort.

[platinumazure]

Agreed, works for me. I wasn't sure how strongly not-an-aardvark felt about merging this until I saw the approval earlier today. Let's get this in!

[Arcanemagus]

For anyone curious, I used the following codemod to put up PRs adding the URL property to all of the eslint-plugin-* packages listed in eslint-rule-documentation that were on the ESLint v3+ rule format:

'use strict';
const { basename } = require('path');

module.exports = function(fileInfo, api, options) {
  const j = api.jscodeshift;
  const urlBase = options.url || 'https://github.com/mysticatea/eslint-plugin-eslint-comments/blob/master/docs/rules';

  const name = basename(fileInfo.path).replace(/.js$/, '');

  return j(fileInfo.source)
    .find(j.ObjectExpression)
    .filter(path => path.parentPath.node.key && path.parentPath.node.key.name === 'meta')
    .forEach(path => {
      const hasDocs = path.node.properties.some(prop => prop.key.name === 'docs');
      const urlProp = j.property('init', j.identifier('url'), j.literal(`${urlBase}/${name}.md`));
      if (hasDocs) {
        const docs = path.node.properties.find(prop => prop.key.name === 'docs');
        const hasUrl = docs.value.properties.some(prop => prop.key.name === 'url');
        if (hasUrl) {
          return null;
        }
        docs.value.properties.push(urlProp);
      } else {
        const docs = j.property('init', j.identifier('docs'), j.objectExpression([urlProp]));
        j(path).replaceWith(j.objectExpression([docs, ...path.node.properties]));
      }
    })
    .toSource({
      lineTerminator: '\n',
      useTabs: false
    });
};