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 JSDoc lint rules to follow style guide #48

Merged
merged 3 commits into from
Sep 27, 2023
Merged

Conversation

bryan-hoang
Copy link
Member

@bryan-hoang bryan-hoang commented Aug 27, 2023

To enforce the addition of file overview comments & JSDoc comments on
exported symbols.

@bryan-hoang bryan-hoang added the enhancement New feature or request label Aug 27, 2023
@bryan-hoang bryan-hoang self-assigned this Aug 27, 2023
@bryan-hoang bryan-hoang requested review from wiwichips and removed request for Xmader September 6, 2023 13:08
@wiwichips
Copy link
Contributor

wiwichips commented Sep 14, 2023

Unrelated to this MR (also present on main), but this rule causes an error for me: '@distributive/brace-style': 'warn'


Oops! Something went wrong! :(

ESLint: 8.40.0

TypeError: Cannot read properties of undefined (reading 'loc')
Occurred while linting /home/will/git/eslint-config/file2.js:2
Rule: "@distributive/brace-style"
    at areTokensOnSameLine (/home/will/git/eslint-config/node_modules/@distributive/eslint-plugin/lib/rules/brace-style.js:35:49)
    at VariableDeclaration (/home/will/git/eslint-config/node_modules/@distributive/eslint-plugin/lib/rules/brace-style.js:60:18)
    at ruleErrorHandler (/home/will/git/eslint-config/node_modules/eslint/lib/linter/linter.js:1049:28)
    at /home/will/git/eslint-config/node_modules/eslint/lib/linter/safe-emitter.js:45:58
    at Array.forEach (<anonymous>)
    at Object.emit (/home/will/git/eslint-config/node_modules/eslint/lib/linter/safe-emitter.js:45:38)
    at NodeEventGenerator.applySelector (/home/will/git/eslint-config/node_modules/eslint/lib/linter/node-event-generator.js:297:26)
    at NodeEventGenerator.applySelectors (/home/will/git/eslint-config/node_modules/eslint/lib/linter/node-event-generator.js:326:22)
    at NodeEventGenerator.enterNode (/home/will/git/eslint-config/node_modules/eslint/lib/linter/node-event-generator.js:340:14)
    at CodePathAnalyzer.enterNode (/home/will/git/eslint-config/node_modules/eslint/lib/linter/code-path-analysis/code-path-analyzer.js:795:23)

^ if I remove the line, it works. Guessing I'm missing some install?

@bryan-hoang
Copy link
Member Author

bryan-hoang commented Sep 14, 2023

Unrelated to this MR (also present on main), but this rule causes an error for me: '@distributive/brace-style': 'warn'

Oops! Something went wrong! :(

ESLint: 8.40.0

TypeError: Cannot read properties of undefined (reading 'loc')
Occurred while linting /home/will/git/eslint-config/file2.js:2
Rule: "@distributive/brace-style"
    at areTokensOnSameLine (/home/will/git/eslint-config/node_modules/@distributive/eslint-plugin/lib/rules/brace-style.js:35:49)
    at VariableDeclaration (/home/will/git/eslint-config/node_modules/@distributive/eslint-plugin/lib/rules/brace-style.js:60:18)
    at ruleErrorHandler (/home/will/git/eslint-config/node_modules/eslint/lib/linter/linter.js:1049:28)
    at /home/will/git/eslint-config/node_modules/eslint/lib/linter/safe-emitter.js:45:58
    at Array.forEach (<anonymous>)
    at Object.emit (/home/will/git/eslint-config/node_modules/eslint/lib/linter/safe-emitter.js:45:38)
    at NodeEventGenerator.applySelector (/home/will/git/eslint-config/node_modules/eslint/lib/linter/node-event-generator.js:297:26)
    at NodeEventGenerator.applySelectors (/home/will/git/eslint-config/node_modules/eslint/lib/linter/node-event-generator.js:326:22)
    at NodeEventGenerator.enterNode (/home/will/git/eslint-config/node_modules/eslint/lib/linter/node-event-generator.js:340:14)
    at CodePathAnalyzer.enterNode (/home/will/git/eslint-config/node_modules/eslint/lib/linter/code-path-analysis/code-path-analyzer.js:795:23)

^ if I remove the line, it works. Guessing I'm missing some install?

That's fixed by Distributive-Network/eslint-plugin#1 😅. You'll need to link it in to test the config properly. Given that this PR is specifically for the JSDoc rules, it's fine disabling the broken plugin rule for now.

Ideally, the parsing PR lands to make it easier to review this PR.

@wiwichips
Copy link
Contributor

wiwichips commented Sep 14, 2023

@bryan-hoang

playing around with it, can't find any problems

I suppose it doesn't necessarily "enforce the addition of [...] JSDoc comments on
exported symbols". For example this doesn't report any errors

'use strict';
/**
 * @file file.js
 * @author will
 * @date somedate
 */

const a = 0;

exports.imExported = function imExported(arg)
{
  return arg+a;
}

the imExported symbol is exported without a JSdoc


I assume this is fine and probably impossible in eslint anyways - please correct me if I'm confused about anything

otherwise MR looks good!

@bryan-hoang
Copy link
Member Author

bryan-hoang commented Sep 19, 2023

@bryan-hoang

playing around with it, can't find any problems

I suppose it doesn't necessarily "enforce the addition of [...] JSDoc comments on exported symbols". For example this doesn't report any errors

'use strict';
/**
 * @file file.js
 * @author will
 * @date somedate
 */

const a = 0;

exports.imExported = function imExported(arg)
{
  return arg+a;
}

the imExported symbol is exported without a JSdoc

I assume this is fine and probably impossible in eslint anyways - please correct me if I'm confused about anything

otherwise MR looks good!

@wiwichips Thanks for the catch! It looks like I missed an option that's needed to enable checking for those exports.

I've also added a rule to require a description on empty comment blocks so that just running ESLint with the --fix option won't just make the errors go away.

Copy link
Contributor

@wiwichips wiwichips left a comment

Choose a reason for hiding this comment

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

will@bestia:~/git/eslint-config ➡ npx eslint file.js 

/home/will/git/eslint-config/file.js
  10:22  error  Missing JSDoc comment  jsdoc/require-jsdoc

✖ 1 problem (1 error, 0 warnings)
  1 error and 0 warnings potentially fixable with the `--fix` option.

will@bestia:~/git/eslint-config ➡ npx eslint file.js  --fix

/home/will/git/eslint-config/file.js
  10:1  error  Missing JSDoc block description  jsdoc/require-description

✖ 1 problem (1 error, 0 warnings)

Awesome, beautiful errors now when trying to export symbols without js docs and it does not "fix it" by just adding comments above the things and acting like thats fine

LGTM

Copy link
Contributor

@eroosenmaallen eroosenmaallen left a comment

Choose a reason for hiding this comment

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

Love it!
Is there a way it (the JSDoc plugin) can catch people using the VSCode import() syntax that keeps breaking our docs generation?

@bryan-hoang
Copy link
Member Author

Love it! Is there a way it (the JSDoc plugin) can catch people using the VSCode import() syntax that keeps breaking our docs generation?

From perusing the plugin's docs/issues, it doesn't look like it supports linting the typedef-import plugin from better-docs that we use. But after reading through jsdoc/jsdoc#1645, it seems like using https://github.com/polyforest/jsdoc-tsimport-plugin/ instead could net us the best of both worlds. A working build w/ jsdoc & being recognized by editor tooling (i.e., JS/TS LSP stuff for VS Code & friends).

To enforce the addition of file overview comments & JSDoc comments on
exported symbols.
Some properties are unnecessary/unused. Others can be updated to target
latest EMCAScript code. A brace was reformatted to follow the brace
style guide.
@bryan-hoang
Copy link
Member Author

@eroosenmaallen I've opened an MR on the DCP repo as a proof of concept for #48 (comment), as I don't see a quick way of getting ESLint w/ the JSDoc plugin to understand this requirement as a lint rule.

@bryan-hoang
Copy link
Member Author

bryan-hoang commented Sep 27, 2023

@eroosenmaallen I've opened an MR on the DCP repo as a proof of concept for #48 (comment), as I don't see a quick way of getting ESLint w/ the JSDoc plugin to understand this requirement as a lint rule.

Given the above, I think this PR can be merged as is since it doesn't look like a blocking comment.

@bryan-hoang bryan-hoang merged commit 81d8637 into main Sep 27, 2023
@bryan-hoang bryan-hoang deleted the feature/jsdoc-config branch September 27, 2023 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants