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

Configure ESLint to require JSDoc @params etc #3103

Merged
merged 3 commits into from
Dec 21, 2022
Merged

Conversation

colinrotherham
Copy link
Contributor

@colinrotherham colinrotherham commented Dec 15, 2022

This PR follows up #2913 (optional JSDoc) by making @param tags mandatory for methods that use JSDoc

It also configures ESLint excluded files correctly using excludedFiles

Which continues work we started for v4.4.0:

To assist with code reviews I've split these changes into:

  1. Ensure JSDoc blocks are complete #3102
  2. Configure ESLint to require JSDoc @params etc #3103
  3. Various JSDoc + type checking fixes #2987
  4. Enable type declaration guards #3123
  5. Add (optional) type declaration checks in app, src #3104

Resolves #2513

Copy link
Member

@romaricpascal romaricpascal left a comment

Choose a reason for hiding this comment

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

Makes all sense to me 🙌🏻 but I'm quite partial to the JSDoc work already. Might be worth getting a quick check with the team before we enforce adding JSDoc 😄

.eslintrc.js Outdated Show resolved Hide resolved
gulpfile.mjs Outdated Show resolved Hide resolved
src/govuk/all.test.mjs Show resolved Hide resolved
src/govuk/i18n.mjs Show resolved Hide resolved
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3103 December 15, 2022 17:11 Inactive
Base automatically changed from jsdoc-review to main December 16, 2022 09:29
src/.eslintrc.js Outdated
}
}
],
'jsdoc/require-param-description': 'warn',
Copy link
Member

Choose a reason for hiding this comment

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

Looks like the 'warn' value snuck back in or maybe you yere you looking to enable require-param-type instead, as that's what we need for typing?

Copy link
Contributor Author

@colinrotherham colinrotherham Dec 16, 2022

Choose a reason for hiding this comment

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

@romaricpascal Oh I probably misunderstood, it's been moved from .eslintrc.js to src/.eslintrc.js

As in, JSDoc params (and descriptions) are only needed for our user-facing source code for documentation

Think we should have it optional everywhere?

Thanks

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've gone ahead and removed the warnings for @param + descriptions

It's something we can monitor during code reviews. Don't want to hassle ongoing spikes with linting issues

(Also set enableFixer: false so ESLint --fix won't keep adding placeholder @param either)

@colinrotherham colinrotherham changed the base branch from main to element-var-dollar December 16, 2022 14:42
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3103 December 16, 2022 14:42 Inactive
Base automatically changed from element-var-dollar to main December 16, 2022 15:06
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3103 December 19, 2022 21:43 Inactive
Some of our tests files had missed the initial `--fix` to swap all “var” to let or const
Whilst optional elsewhere, for our source directory we’ve flipped the “JSDoc blocks are optional” rules at root

Parameters (+ descriptions) are still optional
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3103 December 20, 2022 17:07 Inactive
@colinrotherham colinrotherham changed the base branch from main to eslint-during-watch December 20, 2022 17:08
Base automatically changed from eslint-during-watch to main December 21, 2022 11:09
Copy link
Contributor

@domoscargin domoscargin left a comment

Choose a reason for hiding this comment

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

Given that the code already meets these settings, I think it makes sense to formalise the jsdoc requirements.

@colinrotherham
Copy link
Contributor Author

Given that the code already meets these settings, I think it makes sense to formalise the jsdoc requirements.

Thanks @domoscargin 👍

@romaricpascal I know you were keen on adding warnings when we've missed type declarations (plus stricter checks for missing @param + descriptions) but only if the team agrees that's what we want

(Need to weigh up the benefits of valid types for publishing versus quick spikes where things change quickly)

So I'll go ahead and merge this one as-is and we can tighten further if wanted in future

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some component JavaScript is complex and difficult to follow
4 participants