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 eslint-plugin-jsdoc for better JSDoc linting #16869

Merged
merged 2 commits into from Aug 7, 2019

Conversation

@dsifford
Copy link
Contributor

commented Aug 2, 2019

Description

This PR is the first part of adding typescript type checking in JS files to the project. This was discussed rather exhaustively in slack, so I'll spare you all the regurgitation of that discussion.

Per @aduth, I'm going to cherry pick the second commit with all the lint fixes and PR that separately so that this PR will be more easily reviewable.

In the meantime, just focus your attention on the first commit.

How has this been tested?

Eslint runs successfully and all errors/warnings that turned up after applying the new plugin were addressed in the second commit. There are no more warnings/errors.

Screenshots

N/A

Types of changes

Build Tooling

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

Closes #13741

Related: https://make.wordpress.org/core/2019/08/01/javascript-chat-summary-july-30-2019/

@dsifford

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

Some things to note

1. There is still some discrepancy with respect to how arrays are documented using the long format.

For example...

/**
 * @param {Array.<string>} foo
 */
// vs
/**
 * @param {Array<string>} foo
 */

As far as typescript is concerned, only the second format is legal (to my knowledge).

It appears that this can be enforced rather simply for arrays, but I'm not sure if you'd also have to add in all other potential uses of this as well (which could get long for other globals (e.g. Map<string,string>, Set<string>, etc.) , and probably impossibly long for TypeScript generics (e.g. MyFoo<string>, MyFoo<string,number,boolean>, etc.).

2. I didn't check uses of {?type} to see if the author actually meant that the value is nullable, or if he/she meant to write this...

/**
 * @param {type} [foo]
 */

... to mean that foo can be either type or undefined (AKA, it's optional). Most of the time from what I can remember when working on the type definitions, the author more than likely meant the latter. That difference is important when enabling typescript for checking.

3. There's no checks to validate order of JSDoc tags in a block, so we'll have to remember to follow a written standardization outlined somewhere else (maybe here if we can expand it).

An example of how this can be problematic as far as style is concerned is that there are @typedefs in the code that I've seen written in the following ways...

/**
 * @typedef {Object} Foobar The Foobar object.
 * @property {string} foo The foo property.
 * @property {number} bar The bar property.
 */

// vs

/**
 * The Foobar object.
 * @typedef {Object} Foobar
 * @property {string} foo The foo property.
 * @property {number} bar The bar property.
 */

// vs

/**
 * @property {string} foo The foo property.
 * @property {number} bar The bar property.
 * @typedef {Object} Foobar The Foobar object.
 */

// vs

/**
 * The Foobar object.
 * @property {string} foo The foo property.
 * @property {number} bar The bar property.
 * @typedef {Object} Foobar
 */

4. There are no checks to validate style with respect to unions. So it is currently possible to get past the lint process with the following two blocks...

/**
 * @param {string|number} foo
 */

// vs

/**
 * @param {(string|number)} foo
 */

I am personally partial to not using parens unless absolutely necessary because it just adds more cognitive overhead when trying to grok the block. Either way, this distinction should probably be documented in the style guide so we remain uniform.


There's probably more I'm forgetting, but let's start there....

@dsifford dsifford force-pushed the add/eslint-plugin-jsdoc branch from 3259970 to 0fe3391 Aug 3, 2019

@dsifford

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2019

Note: I rebased from master after #16870 merged. So now this PR should be good to merge/review.

@ntwb

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

Testing this PR currently:

Running the following tasks passed without issue:

npm test
npm run lint-js
./node_modules/.bin/wp-scripts lint-js

As there are no regressions in testing existing .js files with this PR I'm approving and going to merge this as is.

The 1-4 points listed in the above comments should be addressed in follow up issues and pull requests as required.

@ntwb
ntwb approved these changes Aug 7, 2019
Copy link
Member

left a comment

Fantastic work @dsifford, thanks a bunch 💐

@ntwb ntwb merged commit 766476a into master Aug 7, 2019

1 of 7 checks passed

Filter opened Filter opened
Details
Filter opened Filter opened
Details
Filter opened Filter opened
Details
Filter opened Filter opened
Details
Milestone It Milestone It
Details
Milestone It Milestone It
Details
Travis CI - Pull Request Build Passed
Details

@ntwb ntwb deleted the add/eslint-plugin-jsdoc branch Aug 7, 2019

@gziolo gziolo added this to the Gutenberg 6.3 milestone Aug 7, 2019

},
requireParamDescription: false,
requireReturn: false,
} ],
'valid-typeof': 'error',

This comment has been minimized.

Copy link
@gziolo

gziolo Aug 7, 2019

Member

Does it mean that es5 config will no longer validate JSDoc comments?

This comment has been minimized.

Copy link
@dsifford

dsifford Aug 7, 2019

Author Contributor

Correct. If that's the intention also, you'd have to have es5.js extend jsdoc.js (and then probably remove jsdoc.js from the top-level, since it already imports es5.js there).

Good catch.

This comment has been minimized.

Copy link
@gziolo

gziolo Aug 8, 2019

Member

Based on the previous implementation and knowing that sometimes we want to explicitly use ES5 I guess you are all correct.

@Aljullu Aljullu referenced this pull request Aug 9, 2019
gziolo added a commit that referenced this pull request Aug 29, 2019
Add eslint-plugin-jsdoc for better JSDoc linting (#16869)
* feat(eslint-plugin): add eslint-plugin-jsdoc for improved jsdoc linting

* chore: fix all JSDoc eslint errors/warnings across the entire repository
gziolo added a commit that referenced this pull request Aug 29, 2019
Add eslint-plugin-jsdoc for better JSDoc linting (#16869)
* feat(eslint-plugin): add eslint-plugin-jsdoc for improved jsdoc linting

* chore: fix all JSDoc eslint errors/warnings across the entire repository
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.