Intent to turn on JSDoc check in ESLint

[lpy]

Tl;dr JSDoc check in eslint will be turnt on by default.

Rules Summary

Explicitly require that

• functions that have a return statement provide an @returns annotation with a type and description, and that
• any JSDoc type signature can be parsed

Explicitly not require that

• JSDoc comments appear for every class or function, or that
• JSDoc comments specify a description, since this should be discretionary, and is often unnecessary if functions are named properly, or that
• every parameter is described, by the same reasoning

eslint rule

"valid-jsdoc": [
  "error", {
    "requireReturn": false,
    "requireReturnType": true,
    "requireParamDescription": false,
    "requireReturnDescription": true
  }
],

There is a bunch of violation in catapult codebase right now, I put them in a spreasheet

How you can help as an owner/reviewer

• Pick items from the spreadsheet and correct the JSDoc
• Force contributors to use correct JSDoc before LGTM their CLs
• Use the valid JSDoc from now

What you should do as a contributor

• If you document your code in the form of JSDoc, make sure you can pass the check by applying the rule above and running ./common/eslint/bin/run_eslint --path YOUR_CODE_PATH

[lpy]

@benshayden @eakuefner @fmeawad @tdresser @zeptonaut @anniesullie

[tdresser]

From: dashboard/dashboard/elements/alert-remove-box.html, with that rule applied:

/usr/local/google/home/tdresser/chromium/src/third_party/catapult/dashboard/dashboard/elements/alert-remove-box.html
  48:7  error  Missing JSDoc for parameter 'event'   valid-jsdoc
  48:7  error  Missing JSDoc for parameter 'detail'  valid-jsdoc

Aren't we trying to not require that every parameter be annotated? "requireParamDescription": false, only makes it so you don't need a description as part of your @param annotation. You still need the annotation itself...

[tdresser]

As one data point, in tracing/tracing/metrics, we have

• 4 cases where the jsdoc refers to a param by the wrong name (probably forgetting to update the jsdoc when updating parameter names)
• 2 syntax errors
• 26 missing returns, 1 missing return description
• 109 missing parameters

[lpy]

mmmm, I thought we were saying we wouldn't require description for every param but still need you to annotate it. @eakuefner could you clarify it? I didn't find a configuration that would allow us to skip params.

[tdresser]

I agree that there's no config option that would let us skip params.

[eakuefner]

What we came to in the meeting was that people shouldn't be required to specify params, but yes, as folks are saying, there does not seem to be an option for disable param checking entirely. We have two options here:

1. Back off on making params optional, and just say that if you do write a JSDoc comment you have to specify params, or
2. File an eslint bug/modify the rule to add a true-by-default requireParams option that, if false, disables param checking.

[lpy]

Let the first option becomes our workaround, and the second option should be the right way, I will file a bug on eslint.

[lpy]

It turns out there's already a feature request that got closed because they claimed they couldn't reach consensus. I am trying to see if we can reopen it.

[zeptonaut]

@lpy I'm pretty sure that the comment that alberto made on the thread you linked to was automatically added after a long period of no discussion on the thread: I've seen the same message posted on other bug threads. I think it's very likely that, if we were to open a bug describing exactly the feature that we'd like to support, link to the old bug that didn't reach consensus, and make it clear that we were willing to write the pull request required to add the feature, the eslint folks would probably be receptive.

[lpy]

feature request submitted.

[zeptonaut]

@lpy, it's probably worth mentioning in the feature request not just what we'd like changed but also why we feel that it's a valid way to use jsdoc.

I think one thing that's important to convey is for a function like:

/**

• @returns {*} The only element of an iterable.
• @throws {InvalidArgumentException} When the element has fewer or greater than one element. */ function getOnlyElement(iterable) { ... }

We don't think that it adds anything to the jsdoc by adding an @param for iterable. Especially given the fact that we're not parsing the jsdocs to build documentation online, but rather using them to help increase readability int he code, we feel that adding parameter descriptions where they're not necessary actually hurts readability.