xojs / xo

❤️ JavaScript/TypeScript linter (ESLint wrapper) with great defaults
MIT License
7.7k stars 289 forks source link

Add --jsdoc CLI option to enable jsdoc linting #282

Closed simonepri closed 6 years ago

simonepri commented 6 years ago

Proposal

I think that it would be a great addition to let users enable this essential ESLint feature easily, without worrying about adding huge rules inside the package.json.

Example usage

Just pass the flag

xo --jsdoc

or enable it via package.json

"xo": {
  "jsdoc": true
}

Rules added

ESlint additional config when the option is provided

"xo": {
  "space": true,
  "plugins": ["jsdoc"],
  "rules": {
    "jsdoc/newline-after-description": "warn",
    "jsdoc/require-description-complete-sentence": "warn",
    "jsdoc/require-hyphen-before-param-description": "warn",
    "require-jsdoc": [
      "warn", {
        "require": {
          "FunctionDeclaration": true,
          "MethodDefinition": true,
          "ClassDeclaration": true,
          "ArrowFunctionExpression": true,
          "FunctionExpression": true // Currently throws an exception see https://github.com/eslint/eslint/issues/9825
        }
      }
    ],
    "valid-jsdoc": ["warn", {
      "requireReturn": false,
      "prefer": {
        "arg": "param",
        "argument": "param",
        "arguments": "extends",
        "const": "constant",
        "defaultvalue": "default",
        "desc": "description",
        "exception": "throws",
        "file": "overview",
        "fileoverview": "overview",
        "fires": "emits",
        "func": "function",
        "prop": "property",
        "return": "returns",
        "virtual": "abstract",
        "yeld": "yelds"
      },
      "preferType": {
        "array": "Array",
        "Boolean": "boolean",
        "date": "Date",
        "Number": "number",
        "object": "Object",
        "regex": "RegExp",
        "String": "string"
      }
    }]
  }
}

Let's discuss about the defaults, and then in case I will open a PR.

JSDoc related eslint plugins and rules:

eslint-plugin-jsdoc by @gajus require-jsdoc by @eslint valid-jsdoc by @eslint

sindresorhus commented 6 years ago

I don't use JSDoc, but I'm open to it. @pvdlg Thoughts?

simonepri commented 6 years ago

Thanks @sindresorhus!

BTW the ESLint's JSDoc support is not perfect, here I've highlighted some improvements that can be done. I've tried to collect all of them inside the rules posted before.

pvdlg commented 6 years ago

So there is 3 different things in this proposal.

valid-jsdoc rule This trigger errors only when a JSDoc bloc is defined and invalid invalid. It let the user choose to not write JSDoc for some functions for example, and to write some for only complex ones etc... It verifies stuff that is part of the JSDoc spec or wildly seen as a the right way to write it. We should keep it configuration loose enough to works for everyone, so I would use the following config:

It's would be a good idea to add this rule to eslint-config-xo as it's non intrusive (it applies only to functions/object that already have a JSDoc, it just force to write it properly).

eslint-plugin-jsdoc plugin The Eslint core valid-jsdoc rule check that the JSDoc follow the spec and is parsable, while the ones in eslint-plugin-jsdoc go beyond that, verifying things like punctuation and spaces.

I experimented with this plugin a few months ago and the rules are way too specific and opinionated for a default configuration like XO. This plugin is useful only for users who have very specific requirement for their JSDoc which is not the case for most XO users. In addition, being too restrictive on the JSDoc format might discourage contributor to write some.

The few users who might need rules from this plugin, probably need them with different configuration. They should create shareable config and use it with XO.

require-jsdoc rule and jsdoc flag As the valid-jsdoc rule is used when a JSDoc bloc is defined, in the end the jsdoc flag would just enable the require-jsdoc rule forcing to write JSDoc for every declaration in the code.

Enforcing 100% JSDoc is not really something very common from what I see in the open source community (AFAIK ESLint is the only major project with near 100% JSDoc coverage).

In addition, it would be difficult to find the best default config for the rule. Some might want a warning, some an error. Some might want a JSDoc block for function expression and declaration, some for declaration only etc...

So I'm not sure adding a flag is justified for a case that is not really common, and requires additional config anyway. It's probably better to just set the rule in XO:

{
  "xo": {
    "rules": {"require-jsdoc": ["warn", {...}]}
  }
}

It can also be added to shareable config along with some eslint-plugin-jsdoc rules.

simonepri commented 6 years ago

About valid-jsdoc I agree with your considerations but I would keep also the preferType configs for 3 main reasons:

About eslint-plugin-jsdoc plugin Just like before the warning for the 3 rules I've mentioned:

"jsdoc/newline-after-description"
"jsdoc/require-description-complete-sentence"
"jsdoc/require-hyphen-before-param-description"

are automagically fixed by xo and they guarantee that the documentation style is consistent. I don't see them more different than rules like capitalized-comments that are default on xo. They are restrictive but they improve the quality of the code and a simple xo --fix resolves all of them. In my opinion they should be enabled by default as well inside eslint-config-xo. After all the aim of xo is to enforce a consistent code style, and the JSDoc is part of the code style so taking a decision on which it should or it not should be an hypen before the param description, for example, it makes sense to me.

About require-jsdoc Yes you are right it would be difficult to find the best default configs. The idea was to let user choose 2 levels of "strictness"

simonepri commented 6 years ago

Summing up using your same comment structure:

valid-jsdoc rule

jsdoc/newline-after-description rule (autofix) If params are documented leave a newline between the function description and the parameter declaration.

jsdoc/require-description-complete-sentence rule (autofix) If a description is provided then:

jsdoc/require-hyphen-before-param-description rule (autofix) If a description for a parameter is provided then require a hyphen between the parameter name and his description.

About the jsdoc rules maybe we can adapt the code from eslint-plugin-jsdoc and insert them inside eslint-plugin-unicorn.

pvdlg commented 6 years ago

I'm experimenting a bit with the 3 new rules: jsdoc/newline-after-description, jsdoc/require-description-complete-sentence and jsdoc/require-hyphen-before-param-description. They seems extremely buggy to says the least.

A few examples:

/**
 * Sentence with a word stating with a
 * Capital letter.
 */

Returns an error, even though it should be valid to break the line and sometimes the break happen before a capitalize word.

/**
 * List of things:
 * - elem 1
 * - elem 2
 */

Returns an error even though doing a list should be perfectly valid in JSDoc.

/**
 * Code example:
 * ```js
 * console.log(test)
 * ```
 */

Once again returns an error even though perfectly valid.

That's just a few examples but I found dozen of similar cases. Basically all the following situation might create an issue:

@simonepri do you have experience with this plugin? Do you use it in your projects? All the things I mentioned really look like bugs to me. And if not bug they are way too restrictive.