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.

Sign up for GitHub

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
Merged

Add eslint-plugin-jsdoc for better JSDoc linting #16869

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5e69ff7
feat(eslint-plugin): add eslint-plugin-jsdoc for improved jsdoc linting
dsifford Jul 31, 2019
0fe3391
chore: fix all JSDoc eslint errors/warnings across the entire repository
dsifford Aug 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 87 additions & 0 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions packages/eslint-plugin/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
### Breaking Changes

- The [`@wordpress/no-unused-vars-before-return` rule](https://github.com/WordPress/gutenberg/blob/master/packages/eslint-plugin/docs/rules/no-unused-vars-before-return.md) has been improved to exempt object destructuring only if destructuring to more than one property.
- Stricter JSDoc linting using [`eslint-plugin-jsdoc`](https://github.com/gajus/eslint-plugin-jsdoc).

### New Features

Expand All @@ -12,6 +13,7 @@
### Improvements

- The recommended `react` configuration specifies an option to [`@wordpress/no-unused-vars-before-return`](https://github.com/WordPress/gutenberg/blob/master/packages/eslint-plugin/docs/rules/no-unused-vars-before-return.md) to exempt React hooks usage, by convention of hooks beginning with "use" prefix.
- The plugin now uses [`eslint-plugin-jsdoc`](https://github.com/gajus/eslint-plugin-jsdoc), rather than the `valid-jsdoc` rule, for more reliable linting of JSDoc blocks.

## 2.3.0 (2019-06-12)

Expand Down
24 changes: 0 additions & 24 deletions packages/eslint-plugin/configs/es5.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,30 +73,6 @@ module.exports = {
'!': true,
},
} ],
'valid-jsdoc': [ 'error', {
prefer: {
arg: 'param',
argument: 'param',
extends: 'augments',
returns: 'return',
},
preferType: {
array: 'Array',
bool: 'boolean',
Boolean: 'boolean',
Copy link
Member

@aduth aduth Dec 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we lose most of these preferred types and tags with the transition? I only see "Object" type being enforced in the current configuration, and return and yield tags:

gutenberg/packages/eslint-plugin/configs/jsdoc.js

Lines 72 to 78 in da54d11

preferredTypes: {
object: 'Object',
},
tagNamePreference: {
returns: 'return',
yields: 'yield',
},

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This something I did?

I may have deleted those that were set to the default value. Hindsight, that may have been wrong because doing that may have loosened the enforcement.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I guess it may work just as well still. I haven't tested all of the previous values, but I do see a warning (as expected) when trying to capitalize a {String} type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/gajus/eslint-plugin-jsdoc#default-preferred-aliases

float: 'number',
Float: 'number',
int: 'number',
integer: 'number',
Integer: 'number',
Number: 'number',
object: 'Object',
String: 'string',
Void: 'void',
},
requireParamDescription: false,
requireReturn: false,
} ],
'valid-typeof': 'error',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

'vars-on-top': 'error',
'wrap-iife': 'error',
Expand Down
29 changes: 29 additions & 0 deletions packages/eslint-plugin/configs/jsdoc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
const globals = require( 'globals' );

module.exports = {
extends: [
'plugin:jsdoc/recommended',
],
settings: {
jsdoc: {
preferredTypes: {
object: 'Object',
},
tagNamePreference: {
returns: 'return',
yields: 'yield',
},
},
},
rules: {
'jsdoc/no-undefined-types': [ 'warning', {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any desire or intention to eventually increase this severity to "error" ? I understand there's a number of existing issues which would need to be resolved first, but my experience with "warning" is that they're often overlooked (in many cases by lack of installed tooling on the part of new contributors) and that the number of issues present can still trend upward.

(Noting this also applies to every rule defined in jsdoc/recommended)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I brought the number to 0 last week, it's up to 2 now. We should enforce the error level everywhere even if this requires a major version update for the plugin.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For tracking, I created an issue at #18458

// Required to reference browser types because we don't have the `browser` environment enabled for the project.
// Here we filter out all browser globals that don't begin with an uppercase letter because those
// generally refer to window-level event listeners and are not a valid type to reference (e.g. `onclick`).
definedTypes: Object.keys( globals.browser ).filter( ( k ) => /^[A-Z]/.test( k ) ),
} ],
'jsdoc/require-jsdoc': 'off',
'jsdoc/require-param-description': 'off',
'jsdoc/require-returns': 'off',
},
};
1 change: 1 addition & 0 deletions packages/eslint-plugin/configs/recommended.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ module.exports = {
require.resolve( './custom.js' ),
require.resolve( './react.js' ),
require.resolve( './esnext.js' ),
require.resolve( './jsdoc.js' ),
],
env: {
node: true,
Expand Down
2 changes: 2 additions & 0 deletions packages/eslint-plugin/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,11 @@
"main": "index.js",
"dependencies": {
"babel-eslint": "^10.0.1",
"eslint-plugin-jsdoc": "^15.8.0",
"eslint-plugin-jsx-a11y": "^6.2.1",
"eslint-plugin-react": "^7.12.4",
"eslint-plugin-react-hooks": "^1.6.0",
"globals": "^12.0.0",
"requireindex": "^1.2.0"
},
"publishConfig": {
Expand Down