require-jsdoc exemptEmptyFunctions and class constructors

[homer0]

Expected behavior

A class constructor without parameters shouldn't require a JSDoc comment block.

Actual behavior

Since the default value of require-jsdoc.exemptEmptyFunctions is false, functions/methods with no parameters or no return require a JSDoc block, but class constructors with no parameters will also trigger the rule, and most of the times it may no be necessary.

ESLint Config

'jsdoc/require-jsdoc': ['error', {
  require: {
    ArrowFunctionExpression: true,
    ClassDeclaration: true,
    ClassExpression: true,
    FunctionDeclaration: true,
    MethodDefinition: true,
  },
}],

ESLint sample

/**
 * It handles something.
 */
class Something {
  constructor() {
    this._doSomePrivateStuff();
  }
}

Environment

• Node version: 10.20
• ESLint version 7.3.1
• eslint-plugin-jsdoc version: 28.6.1

---

I think this can be easily fixed by modifying this validation:

- if (exemptEmptyFunctions && isFunctionContext) {
+ if (isConstructor(node) || (exemptEmptyFunctions && isFunctionContext)) {

I couldn't figure out how to pull the isConstructor from the iterator utilities, so just for the sake of testing it, I copied the function into the requireJsdoc.js file, modified the if and everything worked as a charm:

// This one doesn't trigger the rule.
/**
 * It handles something.
 */
class Something {
  constructor() {
    this._doSomePrivateStuff();
  }
}
// This one triggers the rule.
const something = () => {
  // eslint-disable-next-line on-console
  console.log('woo';
}

Thanks!

[brettz9]

I've now added isConstructor to jsdocUtils. However, I think we can instead use a new method I extracted out of avoidDocs used by methods like require-returns (i.e., exemptSpeciaMethods) which has the isConstructor check which will allow exemption for constructors if an option checkConstructors is not set to true (since normally, some may wish to force checks on them). This approach also allows us to add checkGetters and checkSetters options which, as with other methods, could allow the method to be disabled on them if the user opts in to these options (without need for coming up with AST for the contexts option)--though I think we would instead want the block to look like this:

      // Allow getter/setters to be exempted
      if (jsdocUtils.exemptSpeciaMethods(...)) {
        return;
      }
      // Only if the option is enabled, prevent empty regular functions and constructors from needing jsdocs
      if (exemptEmptyFunctions && (isFunctionContext || jsdocUtils.isConstructor())) {
        const functionParameterNames = jsdocUtils.getFunctionParameterNames(node);
        if (!functionParameterNames.length && !jsdocUtils.hasReturnValue(node, context)) {
          return;
        }
      }

[brettz9]

But I think changing the default of exemptEmptyFunctions may be a separate issue though one I am personally open toward.

[homer0]

If I'm reading it right, with exemptSpeciaMethods you skip de validation entirely and what I was looking for was the functionality of exemptEmptyFunctions on constructors with no parameters:

• If a constructor has a parameter, it should have a JSDoc block.
• If a constructor doesn't have any parameters, the JSDoc block should be optional (not validated).

Using exemptSpeciaMethods with constructors would cause an early exit and the validation of whether the method has parameters or not would be skipped.

If you think changing exemptEmptyFunctions is a good idea, what about adding something like exemptEmptyConstructors with a default of true? I believe constructors are a special case because you know what the method is for and you know the return value.

Btw, thanks for the fast reply :D!

[brettz9]

There are two changes in the code above:

1. The jsdocUtils.exemptSpeciaMethods() checks for the options checkConstructors, checkGetters, and checkSetters, and if any of those constructs exist when the options are true, it will exit early and not report. Note that this checkConstructors is for disabling the need for documenting constructors entirely, even if they have parameters. This change is, I know, not really related to your request but I mention it as it is somewhat on topic (and would be a natural addition given how it fits with the preparatory refactoring I have done for your desired change anyways). The idea here is to bring parity with some of our other rules which have these options.
2. if (exemptEmptyFunctions && (isFunctionContext || jsdocUtils.isConstructor())) {. This differs from your proposal in not automatically disabling checks for constructors (some projects might always wish constructors to be documented). It does change the existing behavior too in that if exemptEmptyFunctions is true, constructors will be exempted as well. However, I do see you may not want to exempt empty functions but do want to exempt empty constructors, so exemptEmptyConstructors (with a default of true) does seem to be a good idea. (This discussion also raises the question of whether all methods (including constructors) should be exempted by exemptEmptyFunctions; while exempting non-param constructors makes sense, I would personally not want to exempt non-param empty functions (unless completely empty), so I'm not sure whether people who want this option would also want it to apply to methods as well. I guess they would since the logic is apparently that "I am only interested in documenting if there are params and return values". But that can be another issue if someone wants it.)

So I guess the code block may instead become:

// For those who have options configured against ANY constructors (or setters or getters) being reported
if (jsdocUtils.exemptSpeciaMethods(...)) {
  return;
}

if (
  // Avoid reporting param-less, return-less functions (when `exemptEmptyFunctions` option is set)
  (exemptEmptyFunctions && isFunctionContext) ||
  // Avoid reporting  param-less, return-less constructor methods  (when `exemptEmptyConstructors` option is set)
  (exemptEmptyConstructors && jsdocUtils.isConstructor())
) {
  // Code to check the function/method is indeed param-less/return-less
}

Sound good?

@golopot, it'd be good to have your input here before I prepare a PR, if you have time these days to review.

[homer0]

I agree with “1.”, it aligns with the other rules’ options and makes sense. Regarding the “2.”, yes, I didn’t consider people who would want to enforce descriptions on constructors even when there are no parameters, so the `excemptEmptyConstructors` makes more sense now. I also agree with your personal opinion about not exempting non-param empty functions, I always try to document everything, but I don’t like writing unnecessary comments either: this was forcing me to write “Class constructor” as description just to keep the option as false and not trigger it. The change looks good to me :D. Thanks!! (I’m trying to incorporate the plugin to the defaults of the configurations I use for my personal projects :D)

…

On Thu, Jul 2, 2020 at 9:40 PM Brett Zamir ***@***.***> wrote: There are two changes in the code above: 1. The jsdocUtils.exemptSpeciaMethods() checks for the options checkConstructors, checkGetters, and checkSetters, and if any of those constructs exist when the options are true, it will exit early and not report. Note that this checkConstructors is for disabling the need for documenting constructors entirely, even if they have parameters. This change is, I know, not really related to your request but I mention it as it is somewhat on topic (and would be a natural addition given how it fits with the preparatory refactoring I have done for your desired change anyways). The idea here is to bring parity with some of our other rules which have these options. 2. if (exemptEmptyFunctions && (isFunctionContext || jsdocUtils.isConstructor())) {. This differs from your proposal in not automatically disabling checks for constructors (some projects might always wish constructors to be documented). It does change the existing behavior too in that if exemptEmptyFunctions is true, constructors will be exempted as well. However, I do see you may not want to exempt empty functions but do want to exempt empty constructors, so exemptyEmptyConstructors (with a default of true) does seem to be a good idea. This discussion also raises the question of whether all methods (including constructors) should be exempted by exemptEmptyFunctions. While exempting non-param constructors makes sense, I would personally not want to exempt non-param empty functions (unless completely empty), so I'm not sure whether people who want this option would also want it to apply to methods as well. I guess they would since the logic is apparently that "I am only interested in documenting params and return values". But that can be another issue if someone wants it. So I guess the code block may instead become: // For those who have options configured against ANY constructors (or setters or getters) being reportedif (jsdocUtils.exemptSpeciaMethods(...)) { return;}if ( // For param-less, return-less functions (exemptEmptyFunctions && isFunctionContext) || // For param-less, return-less constructor methods (exemptyEmptyConstructors && jsdocUtils.isConstructor())) {} Sound good? @golopot <https://github.com/golopot>, it'd be good to have your input here before I prepare a PR, if you have time these days to review. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#600 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABVATPVOKJ6ZOEVHUSSXMH3RZUSI5ANCNFSM4OOSHM5Q> .

[brettz9]

(I’m trying to incorporate the plugin to the defaults of the configurations I use for my personal projects :D)

:-) We tend to be open to the possibility of breaking changes, preferring to have things make better sense, so feel free to suggest any other default changes that may appeal to others.

[homer0]

Great :D! And don't worry, I also maintain a few projects, so I know what it means respecting semver :P.

I found another "issue", super unrelated to this one, and I can fix it using the plugin settings, so you tell me if I should create a new issue or not: check-types tells me that the default type for Object should be (lowercase) object, but the JSDoc documentation says it should be Object (third row).

On the same page, if you go to the bottom, there's a typedef with lowercase object, but if you go to the typedef page, the examples are with the uppercase Object :P.

The VSCode intellisense doesn't recognises object.<string,string>, it says any; changing it to object<string,string> makes the type {}, but if I use Object.<string,string>, it properly shows that the key is a string and the value too.

I know VSCode uses the TS-Server behind the curtains and that on TS, Object !== object; but if we ignore the TS-Server, if I follow the examples from the JSDoc page, it works.

Maybe the preferred type for Object should be Object, what do you think?

[brettz9]

I found another "issue", super unrelated to this one, and I can fix it using the plugin settings, so you tell me if I should create a new issue or not: check-types tells me that the default type for Object should be (lowercase) object, but the JSDoc documentation says it should be Object (third row).

On the same page, if you go to the bottom, there's a typedef with lowercase object, but if you go to the typedef page, the examples are with the uppercase Object :P.

Yeah, jsdoc comes across more fundamentally I think more as a documentation tool than a precise specification even while it has served as a specification for other tools or specifications that have built on top of it. Of course, it is good to follow it, but the jsdoc tool is often more forgiving than the docs, and the docs are not always clear. For jsdoc type parsing, it is a bit more clear, as there is jsdoc's catharsis which has a formal grammar. But this doesn't cover the jsdoc tags.

The VSCode intellisense doesn't recognises object.<string,string>, it says any; changing it to object<string,string> makes the type {}, but if I use Object.<string,string>, it properly shows that the key is a string and the value too.

I know VSCode uses the TS-Server behind the curtains and that on TS, Object !== object; but if we ignore the TS-Server, if I follow the examples from the JSDoc page, it works.

I do recall they are different in TS but don't recall the differences (and being behind the GFW, I can't easily Google now).

Maybe the preferred type for Object should be Object, what do you think?

If this is more suitable for TS, we can change it for "typescript" mode (which is now auto-set when the @typescript-eslint package parser is detected). Feel free to file a PR for that (linking back to your comment here starting this subthread), preferably with links to TS specs or docs on the differences between the two in TS.

As far as changing for non-TS users, we had previously actually used "Object" but switched to "object" for the reasons outlined at https://github.com/gajus/eslint-plugin-jsdoc#why-not-capital-case-everything .

[homer0]

I'm not a TypeScript user, I mentioned it because VSCode uses the TS-Server to pick JSDoc comments for the intellisense and that's was kind of the reason for me wanting to "fix and normalize" all my projects' documentation.

I'm not sure if changing the default preferred type for the TypeScript mode would be the best, as you said it, there's a difference: Object refers to the instances of Object and object is the type of all non-primitive values. It's not one or the other, they are used for different cases.

During the day (it's 5AM), I'll ask some friends that work with TypeScript for their opinion, because changing it "may not be the best solution", but at the same time, it may be irrelevant for TS users, as I believe they usually use @interface whenever they write "dictionaries".

I'll let you know, and depending on what they tell me, I may send the PR during the weekend.

Thanks!!

[homer0]

I’m an idiot, I spent so much time looking for the differences between Object and object that I skipped the TypeScript examples from the handbook.

If you want to type a dictionary, you use Object:

/**
 * A map-like object that maps arbitrary `string` properties to `number`s.
 *
 * @type {Object.<string, number>}
 */
var stringToNumber;

I’ll try to send the PR during the weekend.

Another issue I’m having, even more unrelated than the previous, is with memberof:

I have a @module node/logger and I want to use it on the @memberof of a @typedef; without brackets (JSDoc way), trigger check-valid-types because I’m using TypeScript mode and it requieres brackets (yes, I wanted to get the obvious mistake out of the way first :P), but if I add brackets, it says that it doesn’t have a type.

This is the syntax I’m using:

/**
 * @module node/logger
 */

/**
 * @typedef {Object.<string,string> Somethig
 * @memberof {module:node/logger}
 */

I didn’t research it a lot, it took me some time to realize I needed the brackets for TypeScript, and when I found the second error I had to leave the computer for a couple of hours :P.

Any ideas?

Thanks!

[brettz9]

Re: module:node/logger, per #145 (comment) , TypeScript is "unlikely to ever support the standard jsdoc import syntax (module:foo/bar.event:MyEvent)." Also of possible interest, the TS docs on what it currently supports: https://www.typescriptlang.org/docs/handbook/jsdoc-supported-types.html (some issues are filed on what it has yet to support). When parsing in TS mode, this syntax is not supported in our underlying type parser, jsdoctypeparser, and therefore not in eslint-plugin-jsdoc.

See #601 re: a Discord chat I started for jsdoc-related discussions. No promises I can reply to all, but should be a good place for such questions, I'd think.

[brettz9]

And thanks for the info on Object/object!

[homer0]

Great about the discord, I'll join tonight :D.

Regarding the JSDoc import, I know TS doesn’t support it and I’m already using {import('…'), the issue is when I want to set the module:… on @memberof, I can’t write it in anyway without getting a validation error: without brackets gets me an error from the parser, and with brackets an error from the plugin.

(I’m probably missing the point that the @memberof represents an import and that’s what you were trying to explain me, right? :P)

[brettz9]

Not sure I fully get you, but:

1. module: style paths are not supported in TS and therefore is not supported in our "typescript" settings.jsdoc.mode (which is now auto-set when @typescript-eslint's parser is detected to be in use). This applies across tags--wherever we expect a type (in brackets, e.g., @param {module:path}) or namepath (e.g., @memberof some/Name/Path) to be present (actually, our plan is to prevent module:-style namepaths even further where only a generic name or namepath makes sense, e.g., in @param names).
2. import() is not supported in settings.jsdoc.mode "jsdoc"; it is supported in TS and therefore in our "typescript" mode.

If you want to mix-and-match your syntaxes--not recommended--you can set your mode to "permissive".

If your parser is TypeScript-based (even if you're using for JS), I guess you should be using "typescript" mode.

If TypeScript expects @memberof types to be in brackets, then we should adjust our code accordingly, but I see microsoft/TypeScript#7237 is still an open issue to even add support for @memberof. As far as jsdoc usage, per https://jsdoc.app/tags-memberof.html , there is no use of brackets. It could also be an issue with your parser. For further discussions--unless you come to a resolution you want to share--let's move this to chat or to a new issue.

[homer0]

(Sorry for the short answer, I'm on the phone)

I’m using the typescript mode and using the import syntax that’s not supported by JSDoc , both the linter and vscode accept it, and for the site generation I wrote a small plugin that removes those lines.

When I said that typescript required the brackets wasn’t because I checked the docs, my fault (sorry); I ran the parser with the code and setting the mode to typescript and it didn’t generate any error, but the plugin does an extra validation that fails (sorry I can’t remember the error, it was something like it was missing the type).

[brettz9]

Feel free to file an issue with the code that generates the issue and the specific rule that gets reported.

[homer0]

Sorry, I couldn't make the PR; I've believe I identified where the modification should be made, but the only way I could fix it would be with an awful patch and I would reject it if were to review it :P.

• The validation I'm looking for happens in here, right?
• The validation is against strictNativeTypes, which is a constant defined outside the context of the function.
• The constant doesn't know about modes, so I should make it aware by extracting the setting and creating the constant inside the function(?).
• I don't like that solution, because for tagNames, you have it on a special file, separated for modes... so the solution should be something similar to that, or we would end up with two ways to manage mode-based-preferences.
• Maybe I should create something like defaultSettingsByMode or similar (are there more rules that can benefit from this?).

All of sudden, I went from wanting to add an if for a value to a big refactor on a project I have almost no experience with it :P, sorry. I believe tomorrow I'll have an hour or two (not sure), so I can give it a second try... can you give me some pointers on how would you do it? If I end up sending the PR, I would like to be something that would be useful for you, the maintainers, and not hotfix you'll have to get used to :P.

Regarding the other things, I'll give you a little bit more context on what I was trying to achieve so you could better understand my crazy messages:

I'm migrating from ESDoc, so I wanted to use proper JSDoc (thus the use of this plugin) for comments and for site generation... and at the same time, I wanted to use it on a way that the VSCode's TS Server would pick my types for the intellisense, so, yes... I'm mixing a little bit of JSDoc with TypeScript.

To import definitions, I'm using @tag {import('./file.js').Type} Type and that works great on VSCode, but it doesn't when I generate the site with JSDoc... so I wrote a small plugin that would take those comments and replace them with empty lines before parsing the files for the site.

Then I wanted to group different things from a same file under the same module on the generated site, that's why I started using @module and @memeberof: the JSDoc parser for the site required module:, but that can't be used with the typescript mode: without brackets, jsdoctypeparser would throw an error; and with brackets, it wouldn't recognize it as a type and report an error before even parsing it.

I ended up going the "hack route": Instead of using @memberof module:some/thing, I'm using @memberof module.some/thing and then my plugin is replacing all the modules.(?!exports$) with module:: VSCode is happy, this plugin is happy and the site is generated as expected.

I'm not entirely sure if the issue with memberof is something you should look at, or if it's as you said, the fact that I'm mixing the modes.

Thanks!

[brettz9]

I can prepare a PR myself for #600 (comment)

As far as object vs. Object, it seems we need the ability to allow either of these types without complaint (since jsdoc users may also wish to use both types) and "typescript" mode to default to using that option. I've submitted #603 to handle this.

As far as your latter issue, that is working as intended. If you ever want to use jsdoc for documentation, you will presumably want a jsdoc dialect which can be understood by the parser that does the documentation (I believe you can use TypeScript to document pure JavaScript+jsdoc, but using their dialect).

While some of these parsers may be forgiving of your use of distinct parsers, for example, a regular jsdoc parser for documentation and a TypeScript parser for IDE use, this is not recommended because:

1. Your tools may start complaining about invalid uses. For example, as mentioned, @memberof is simply not supported by TypeScript and module: won't apparently ever be supported by TS. (And jsdoc doesn't support import().)
2. Our own plugin has certain edge cases where it is not clear what being "permissive" should mean.

Nevertheless, if you really want to try to accommodate both, you can set settings: {jsdoc: {mode: 'permissive'}} in your .eslintrc and you may not need to do your plugin hack (at least as far as pleasing our plugin), as we do have a mode with jsdoctypeparser to be more forgiving and try to use the option which works in the most environments.

Going forward, please let us not discussing these unrelated items here further. If you want an extended discussion, please use the chat rooms, or file a new issue. Of course, feel free to add comments here though if related to the original issue.

[gajus]

🎉 This issue has been resolved in version 28.7.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[brettz9]

In addition to the implementation of this feature, there is now also (per 82ca868 ) the ability to selectively enable object and Object (enabled by default in "typescript" mode / with the @typescript-eslint/parser).

To do so in "jsdoc" mode requires this trick (at least until such time as we may settle on some formal options for it):

      settings: {
        jsdoc: {
          preferredTypes: {
            object: 'Object',
            Object: 'object',
          },
        },

Other built-in types with fixed casing can be permitted in a similar manner.

[homer0]

@brettz9 yay! thank you so much :D!!