Skip to content

fix: re-enable jsdoc checks #215

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

Closed
wants to merge 2 commits into from
Closed

fix: re-enable jsdoc checks #215

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
54ec93c
fix: re-enable jsdoc checks
cwillisf Mar 31, 2025
202fee8
fix: disable jsdoc/require-jsdoc
cwillisf Apr 2, 2025
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
24 changes: 24 additions & 0 deletions lib/eslint.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import formatjs from 'eslint-plugin-formatjs'
import html from 'eslint-plugin-html'
import htmlSettings from 'eslint-plugin-html/src/settings.js'
import importPlugin from 'eslint-plugin-import'
import jsdoc from 'eslint-plugin-jsdoc'
import jsxA11y from 'eslint-plugin-jsx-a11y'
import markdown from 'eslint-plugin-markdown'
import react from 'eslint-plugin-react'
Expand Down Expand Up @@ -131,6 +132,29 @@ const makeEslintConfig = ({ tsconfigRootDir, globals: globalsIn } = {}) => {
'import/no-duplicates': 'error', // Forbid duplicate imports
},
},
// eslint-plugin-jsdoc
jsdoc.configs['flat/recommended-error'],
{
files: ['**/*.ts', '**/*.tsx', '**/*.mts', '**/*.cts'],
extends: [jsdoc.configs['flat/recommended-typescript-error']],
},
{
rules: {
// If JSDoc comments are present, they must be informative (non-trivial).
// For example, the description "The foo." on a variable called "foo" is not informative.
// https://github.com/gajus/eslint-plugin-jsdoc/blob/main/docs/rules/informative-docs.md
'jsdoc/informative-docs': ['error'],

// Require JSDoc comments on public / exported items.
// https://github.com/gajus/eslint-plugin-jsdoc/blob/main/docs/rules/require-jsdoc.md
'jsdoc/require-jsdoc': [
'error',
{
publicOnly: true,
},
],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm a little concerned this may be too much of a hassle, even with publicOnly enabled, so I'm open to feedback.

Copy link

@georgyangelov georgyangelov Apr 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I guess the trouble with publicOnly is that it can't know whether something is exported from the package, or is exported from a module but otherwise private in the package 🤔

Maybe make it configurable? I see this being beneficial for library packages (scratch-editor/*, scratch-platform/scratch-web-shared), while for scratch-web it might not make as much sense as it's not being consumed by other code.

Also I would say for TS files we should make it so that type annotations in JSDoc are not mandatory - TS does a better job of that and they'll go out of sync pretty quickly if we have to duplicate them.

I've generally found that if you have TS and name your functions/components/types appropriately - JSDocs rarely provide much in addition. They are useful in cases where the component/function does complicated logic that needs explanation and examples, but that's hard to enforce automatically 🤔

Copy link
Contributor Author

@cwillisf cwillisf Apr 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe make it configurable? I see this being beneficial for library packages (scratch-editor/*, scratch-platform/scratch-web-shared), while for scratch-web it might not make as much sense as it's not being consumed by other code.

Yeah, that's probably best. I'll remove jsdoc/require-jsdoc from this config. A particular project or library can always enable it locally, if desired.

Also I would say for TS files we should make it so that type annotations in JSDoc are not mandatory - TS does a better job of that and they'll go out of sync pretty quickly if we have to duplicate them.

Good news on this front! With this configuration, redundant JSDoc type annotations are forbidden in TS. In other words, after removing jsdoc/require-jsdoc, this configuration will permit three cases:

  • JS/TS with no JSDoc at all
  • JS with JSDoc that includes type annotations (@param {string} foo The thing with the stuff.)
  • TS with JSDoc that excludes redundant type annotations (@param foo The thing with the stuff.)

I included the word "redundant" because there are a few situations in TS where JSDoc type annotations are not redundant and not forbidden. See docs: https://github.com/gajus/eslint-plugin-jsdoc/blob/main/docs/rules/no-types.md

Copy link

Choose a reason for hiding this comment

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

Nice!

},
},
// eslint-plugin-jsx-a11y
jsxA11y.flatConfigs.recommended,
// eslint-plugin-markdown
Expand Down
3 changes: 1 addition & 2 deletions lib/prettier.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,7 @@ const prettierConfig = {
}

/**
* Make a Prettier configuration for Scratch style.
* @returns {import("prettier").Config}
* @returns {import("prettier").Config} A Prettier configuration for Scratch style.
*/
const makePrettierConfig = () => prettierConfig

Expand Down