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

chore: Lint TsDoc. #6353

Merged
merged 21 commits into from
Aug 23, 2022
Merged

chore: Lint TsDoc. #6353

merged 21 commits into from
Aug 23, 2022

Conversation

maribethb
Copy link
Contributor

@maribethb maribethb commented Aug 16, 2022

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide
  • I ran npm run format and npm run lint

The details

Resolves

Resolves #6338

Proposed Changes

  • Adds the eslint-plugin-jsdoc plugin and configures some rules for it. Only on TS files.
  • Had to disable the require-param rule as it breaks for functions that use this: check-param-names - False positives with TypeScript "this" parameter gajus/eslint-plugin-jsdoc#904 (this caused the CI failures as it broke closure. also i reverted the change that automatically added the extra @param this but it wasn't a clean revert so sorry for the messy git history)
  • Disable the check-param-name rule for same reason. if we ever re-enable this rule if above issue is fixed, we should most likely keep checkDestructured as false. we could consider changing how we document destructured params but afaict TsDoc provides no official guidance so i don't see a need to enforce a specific dialect different than we have.
  • Disabled some rules relating to checking types that fail because closure decided @suppress {warningName} made sense even though warningName is not a type in this case. when we remove closure we should both re-enable these rules and remove the closure-specific annotations (e.g. nocollapse, dict, unrestricted, etc)
  • Remove all of the @fileoverview comment blocks. I checked over 100 of them and they were all identical copies of either the class or namespace comment (right above it in most cases). @fileoverview is not a supported tag and these were worthless comments anyway.
  • Fixed the auto-fixable errors which included changing @return to @returns and adding a blank line after the description in a tsdoc block. reverted this change once, not cleanly, and did it again. sorry
  • Removed @struct which is not supported and doesn't do anything anyways because they were on es6 classes which are struct by default.
  • Only require jsdoc on exported functions/etc
  • Added missing jsdoc
  • Silenced some warnings on patched functions

No lint warnings!

Behavior Before Change

No linting on TsDoc.

Behavior After Change

Linting on TsDoc.

Reason for Changes

Cleaning up the TsDoc will improve the API Extractor output.

Additional Information

Microsoft provides an eslint plugin for tsdoc (eslint-plugin-tsdoc) but it works in the most annoying way I can think of - instead of separating the things it checks for into individual rules that you can then disable or modify as you wish, it puts all the checks into one rule called tsdoc/syntax so you cannot enable only some of the checks. it's very all or nothing.

and even though they made the actual TsDoc parser accept parameter docs that don't have hyphens, the lint rule still enforces that you must have hyphens and there is no way to turn that off. VS Code, which MS also makes, doesn't add the hyphens in the autogenerated docs. so this is one area where it seems like the community disagrees with the TsDoc spec and they somewhat accepted that.

anyway you cannot incrementally fix the lint errors like i'm doing with the other lint rules, using the eslint-plugin-tsdoc. and additionally there is no rule that checks that each public function has tsdoc. instead that logic is built into api extractor for some reason.

so instead of dealing with that plugin, i'm just using eslint-plugin-jsdoc which you can configure to typescript "flavor". this is the same plugin we use in samples. it is highly configurable. the default settings do not perfectly match the TsDoc spec (e.g. there are some tags that TsDoc doesn't recognize that the plugin is fine with) but overall it should be easier to use and adapt how we want it.

@maribethb maribethb changed the title Lint tsdoc chore: Lint TsDoc. Aug 17, 2022
@maribethb maribethb marked this pull request as ready for review August 17, 2022 01:16
@maribethb maribethb requested a review from a team as a code owner August 17, 2022 01:16
@maribethb
Copy link
Contributor Author

The remaining 11 warnings should probably all be fixed. They are related to missing or incorrect jsdoc comments. I will take a look at those tomorrow but the rest of this can be reviewed.

@maribethb
Copy link
Contributor Author

p.s. thank you to Beka for the regex wizardry

.eslintrc.json Outdated
"parser": "@typescript-eslint/parser",
"parserOptions": {
"project": "./tsconfig.json",
"tsconfigRootDir": ".",
"ecmaVersion": 2020,
"sourceType": "module"
},
"extends": ["plugin:@typescript-eslint/recommended"],
"extends": ["plugin:@typescript-eslint/recommended", "plugin:jsdoc/recommended"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Line length.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed this, but for future reference
a. this file is not actually linted, and we don't have rules for json files
b. if we follow the rules for js files, line length is 100 and this was only 90

if we want to consistently lint this file we'd need to set that up.

core/block_animations.ts Show resolved Hide resolved
core/contextmenu.ts Outdated Show resolved Hide resolved
core/flyout_base.ts Show resolved Hide resolved
@cpcallen
Copy link
Contributor

I'm going to assign this to Rachel to re-review in my absence.

@cpcallen cpcallen removed their assignment Aug 20, 2022
@cpcallen cpcallen added component: TypeScript PR: chore General chores (dependencies, typos, etc) component: build process labels Aug 20, 2022
@rachel-fenichel
Copy link
Collaborator

LGTM once you fix the line length issue Christopher pointed out--I'm satisfied with your reasons for leaving the other things as-is.

@maribethb maribethb dismissed cpcallen’s stale review August 23, 2022 21:26

Addressed comments, and got fenichel's approval while christopher is ooo

@maribethb maribethb merged commit 037eb59 into google:develop Aug 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: build process component: TypeScript PR: chore General chores (dependencies, typos, etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants