use eslint-plugin-jsdoc

[simonseyock]

This PR introduces eslint-plugin-jsdoc as mentioned in openlayers/openlayers#11960

At the moment I see 3 problems that will block the use of this plugin:

1. multiline object types with indentation are not detected properly gajus/eslint-plugin-jsdoc#686
2. no-undefined-types: false positives inside functions for type parameters declared by template tags gajus/eslint-plugin-jsdoc#559
3. a combination of typeof and import() does not work jsdoctypeparser/jsdoctypeparser#133

This will allow a more rigid checking of the jsdocs. I tried it and it already detected some issues in the codebase, for example missing parameter descriptions or missing @property tags.

There are several stylistic rules that can be configured but will produce many problems in the codebase initially, but can be fixed easily. For example in the codebase @return is used way more often than @returns so we could check for that.

Also this plugin could help to move away from google closure syntax. Mainly @property {Type=} prop which could get replaced by @property {Type} [prop]. Another one is @property {!Type} prop which in my opinion could either get removed or get replaced by "strictNullChecks": true in the tsconfig. This produces 1075 issues though.

[simonseyock]

I rearranged some of the rules so it might be easier to read them. I noticed that only gajus/eslint-plugin-jsdoc#686 really is blocking because the other issues are only blocking rules that were not enforced before and should, at least in the main src/ folder, get checked by tsc anyways. I disabled these rules for now.

[tschaub]

I noticed that only gajus/eslint-plugin-jsdoc#686 really is blocking

Flashback microsoft/TypeScript#26528

[simonseyock]

Wow, yes that seems like a very similar issue. It might be a little easier though as the comment-parser lib does not handle the type parsing on its own.

[simonseyock]

After the changes in openlayers/openlayers#11975 this plugin only produces 2 problems:

   * @param {{includeWhiteSpace: (boolean|undefined),
   *     ignoreElementOrder: (boolean|undefined)}=} options The options.

and

 * @param {Array<{extent: import("./extent.js").Extent,
 *                 image: (HTMLCanvasElement|HTMLImageElement|HTMLVideoElement)}>} sources
 * Array of sources.

both can easily be fixed on openlayers side by using @typedefs. Should I open a PR against openlayers with this? Then we could discuss the rules I proposed in this PR. Afterwards I would tidy up this PR.

Maybe we can remove the @openlayers/valid-tsdoc rule then - but I am not 100% sure if this catches everything that this plugin did before.

[tschaub]

Thanks for the work on this, @simonseyock. I'm in favor of adding @typedefs for those nested types.

I'm also in favor of removing our use of the custom @openlayers/valid-tsdoc rules. This was set up initially because ESLint's valid-jsdoc rule didn't parse all of the annotation type syntax we were using. The conventions it enforced were pretty limited and would be covered by these rules:

• require-returns
• require-returns-type
• require-returns-description
• require-param
• require-param-name
• require-param-type
• require-param-description

Any additional checks that detect missing properties on types etc. would be great.

[simonseyock]

I added the typedefs to openlayers/openlayers#11975

The rules which I think might be interesting but I do not have activated at the moment are:

• check-syntax would only allow one type of syntax: closure or typescript.
• no-undefined-types disallows undefined types but fails because of no-undefined-types: false positives inside functions for type parameters declared by template tags gajus/eslint-plugin-jsdoc#559 . This is only interesting for source outside of the tsc checked folders.
• require-property-description produces 232 problems in the source code
• valid-types is blocked by a combination of typeof and import() does not work jsdoctypeparser/jsdoctypeparser#133 and is also only interesting for source outside of the tsc checked folders.
• check-types ensures that native types are cased uniformly - this could get activated easily. Should we stick to the defaults?

[tschaub]

• check-syntax would only allow one type of syntax: closure or typescript.

+1 for enabling and going with the "typescript" mode

• no-undefined-types disallows undefined types but fails because of gajus/eslint-plugin-jsdoc#559 . This is only interesting for source outside of the tsc checked folders.

We could include this in a future version when the upstream issue is fixed.

• require-property-description produces 232 problems in the source code

I'd say only enable this if you feel like helping address the problems it reveals. I'll contribute as well.

• valid-types is blocked by jsdoctypeparser/jsdoctypeparser#133 and is also only interesting for source outside of the tsc checked folders.

Same above about including in a future version when the blocking issue is addressed.

• check-types ensures that native types are cased uniformly - this could get activated easily. Should we stick to the defaults?

If you see value in this one, go for it. Not clear to me how much would have to change to conform (perhaps just changing Object to object).

[simonseyock]

For check-types I forgot that in TypeScript mode the casing for most types is not reported. But it is possible to forbid the bracket ([]) and dot (.<>) notations and replace them with <>. I pushed a commit for this.

[simonseyock]

If we remove the google closure syntax we lose the ! / not null type annotation though. But I don't know if we benefited much of it before. As mentioned the strictNullChecks of tsc would be an option to enforce null checking or proper annotation as not-optional of parameters. This reports 1074 problems in the code - which I hope is mainly from incorrectly as optional annotated parameters or properties. Many problems are reported multiple times.

I would just remove the non-nullable types for now and I might crawl through the code and try to fix the issues with the nullable types.

[simonseyock]

require-property-description would be good to have for new PRs I think, but for the existing code base I would only copy the property name I must confess.

[simonseyock]

I was actually wrong about the nullable types. I remembered seeing an error earlier, but I was wrong. As the docs state they only flag google closure syntax if it has a meaningful replacement.

[simonseyock]

I found that on some occasions the description of a property is indented and on others it is not.

 * @property {Collection<import("./Overlay.js").default>|Array<import("./Overlay.js").default>} [overlays]
 * Overlays initially added to the map. By default, no overlays are added.

This style would be enforcable by the plugin. Should we use that?

Edit: To be more precise, I only saw the indentation only on nested types so far.

Also there is a rule which enforces to start all descriptions to start with uppercase and end with .. This is used in most descriptions, even if they are only one word long. This rule causes some issues, I would look into them if you think this makes sense.

[simonseyock]

The mentioned require-description-complete-sentence seems not very good as it does also enforce uppercase after each .. For this we would need to configure all used abbreviations. But that does not seem sensible to me. But a simpler rule might be enforcable with match-description, for example every description should start with an uppercase letter and end with a dot.

[simonseyock]

I looked more into match-description. It might be a little over restricting. The source contains descriptions that end with examples or lists, a dot there would be not a very big deal, but not very pretty. Also it has a quite cryptic error message, I opened gajus/eslint-plugin-jsdoc#686 for that, though.

So if we leave this one out, the only rule left might be check-indentation that could enforce no indentation.

[tschaub]

I think rules that improve the quality of the generated docs make sense to add. Those that improve the legibility of the comments in the code are nice, but could get tedious if there are too many - since I’m guessing we don’t get auto fix.

[simonseyock]

Ok, in this case I would consider this PR ready to get merged. Would you mind taking a look at it and releasing a new version of the plugin?

[simonseyock]

@tschaub just wanted to ask if this is ready to merge and release then?

[tschaub]

Thanks, @simonseyock. Published as [email protected].