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

Chore: Update jsdoc plugin and tweak rules in effect #14814

merged 18 commits into from Aug 20, 2021


Copy link

@brettz9 brettz9 commented Jul 20, 2021

Prerequisites checklist

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

[ ] 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
[x] Other, please explain:

What changes did you make? (Give an overview)

  • Update eslint-plugin-jsdoc peer dependenciy in eslint-config-eslint (and update as dev. dep. in eslint)
  • Update .eslintrc.js to disable rules that files with @example would otherwise be wrongly flagged
  • Indicate rules applied from plugin:jsdoc/recommended (but switched to error's), add other helpful jsdoc rules
  • Fix JSDoc per latest JSDoc plugin update

Is there anything you'd like reviewers to focus on?

  • One minor JSDoc change from semi-colon to comma should not be necessary, but waiting on a fix to jsdoc-type-pratt-parser and disabling linter around a few TypeScript constructs which the parser is also awaiting support for
  • The function() : string syntax is not TypeScript-based, so switched to that indicated by TS
  • For the sake of a rule to avoid accidental extra asterisks, switched some initial line asterisks to -

Note that I commented out the following jsdoc rules as they would have entailed more diff noise, and I wasn't sure if you wanted them, or how, but I do think they'd be good to enable:

  • jsdoc/check-indentation - This and the next rule might add noise, and not sure you were consistent, but might be nice to enforce consistency
  • jsdoc/check-line-alignment
  • jsdoc/require-file-overview - You have these in use already, but would need to confine to a directory
  • jsdoc/require-throws - Should be useful to require denoting the types thrown by the function when a throw is found
  • jsdoc/check-values - This rule can enforce SPDX license identifiers; if ok, I could add the other text currently there to a @copyright tag

@eslint-github-bot eslint-github-bot bot added the triage An ESLint team member will look at this issue soon label Jul 20, 2021
lib/linter/report-translator.js Outdated Show resolved Hide resolved
lib/rules/no-loop-func.js Show resolved Hide resolved
Copy link
Contributor Author

brettz9 commented Jul 20, 2021

I see this PR might need to wait on eslint and eslint-config-eslint updating engines to Node 12+...

Copy link

nzakas commented Jul 22, 2021

The engine upgrade won’t happen until the next major version (which will hopefully be soon).

.eslintrc.js Outdated Show resolved Hide resolved
Copy link
Contributor Author

brettz9 commented Jul 29, 2021

Made a number of mostly small, misc. changes.

A few items/questions.

  1. I added the helpful eslint-comments plugin, but it's perhaps most useful rule, eslint-comments/require-description is currently commented out as it would require 136 changes in this repo alone.

This rule requires that a reason be given for disabling the linting--something very helpful both to understand why there was some laxening, and to prevent wasted time of other devs on seeing if the disable directive could be removed.

Let me know if you wish to enable this.

  1. For the rule jsdoc/check-line-alignment, enforcing the "never" vs. "always" style would require 126 vs. 1833 changes. I'd go with "never", but wanted to confirm. In other words, this would not require or even allow this type of extra spacing between docs:
 * My function.
 * @param {string} lorem  Description.
 * @param {int}      sit        Description multi words.

but would instead insist on:

 * My function.
 * @param {string} lorem Description.
 * @param {int} sit Description multi words.
  1. There were 37 files without a @fileoverview. I could enable jsdoc/require-file-overview: "error" or enable on a particular directory if you wanted to ensure each file or each file in a particular directory had this. If you don't care if you forget it yourself or if others do, I can ignore, but I mention as this was a frequent pattern and mentioned in the docs.

  2. jsdoc/require-throws: "error"

I went ahead and enabled this one, fixing 42 errors. Let me know if you want me to revert that commit. It can, of course, only detect when there is a throw statement.

  1. I added some extra enforcement of jsdoc types, e.g., requiring that any documented Promise have a type, that the more TypeScript-like "any" be used over "*" and that the older dotted Object.<something> notation be changed to Object<something>.

One additional change I prefer in my own projects (and currently commented out in the ESLint config) is to prohibit use of a plain Array, Object, or Function, except as a typedef base type. One of the most disappointing experiences in perusing code docs in my experience is finding such a type as it often gives very little idea of what detailed structure the type has and is usually the type one most needs to understand.

In rarer cases, such as with a very general purpose utility, there may truly be no limits on the specific type of array, object, or function that may be returned, but in such cases, one can insist on a few types you might create @typedefs for, such as GenericArray, GenericObject (or PlainObject so as to suggest it is not a host object or need be anything besides a plain literal), and GenericCallback.

Making this change will require a large amount of work, however (36, 398, and 54 fixes, respectively, just in this repo). I could disable this rule for now on the eslint project, but encourage it on other projects by adding it by default to eslint-config-eslint.

Copy link
Contributor Author

brettz9 commented Jul 29, 2021

I've also added a commit for Unicorn, also with a good number of changes.

Copy link

nzakas commented Aug 3, 2021

Our challenge here is that we have a large amount of outside contributors, and we don’t want to put in too many rules that enforce things that aren’t auto fixable. That’s too discouraging for new or infrequent contributors.

So checking alignment will be particularly annoying if it can’t be autofixed. If it can be autofixed, then let’s do it, but if not, I’d prefer not enforcing any type of alignment.

The others seem like good ideas and I’d suggest doing those each as a separate PR so we can deal with any issues that come up in a more focused way.

I’d also prefer we allow Object, Array, etc. I don’t really want a lot of energy spent on figuring out the correct type. Getting people to include JSDoc at all was really hard, I don’t want to throw up any more barriers than necessary

Copy link
Contributor Author

brettz9 commented Aug 18, 2021

This has been rebased, and I think is ready again for review now that master has updated engines. Note, however, that I removed the Unicorn addition here, in part because it is currently failing, and in part because I think the PR was already getting crowded, so I could see about submitting that in a separate PR.

bmish approved these changes Aug 18, 2021
nzakas approved these changes Aug 20, 2021
@brettz9 brettz9 merged commit 58840ac into master Aug 20, 2021
13 checks passed
@brettz9 brettz9 deleted the linting-jsdoc branch Aug 20, 2021
@brettz9 brettz9 added chore This change is not user-facing and removed triage An ESLint team member will look at this issue soon labels Aug 20, 2021
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Feb 17, 2022
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
archived due to age This issue has been archived; please open a new issue for any further discussion chore This change is not user-facing
None yet

Successfully merging this pull request may close these issues.

None yet

4 participants