gajus / eslint-plugin-jsdoc

JSDoc specific linting rules for ESLint.
Other
1.09k stars 157 forks source link

`jsdoc/no-types`: Not reporting violations on TS interfaces #1249

Closed ej612 closed 3 months ago

ej612 commented 3 months ago

Hi there!

I have the following eslint config:

extends: [
  'plugin:jsdoc/recommended-typescript-error'
]

And the following code:

export class A {
    /**
     * @param {string} paramA
     */
    public methodA(paramA: string) {
        // ...
    }
}

no-types is warning me not to use types in my JSdoc (as expected): image

If however, I don't declare a class, but an interface, like so:

export interface B {
    /**
     * @param {string} paramA
     */
    methodB(paramB: string): void
}

The rule no longer reports a violation.

Is this expected behavior? I would expect it to report a violation on interfaces as well.

Thanks a lot in advance!

Environment

brettz9 commented 3 months ago

The defaults for certain rules are to only check 'ArrowFunctionExpression', 'FunctionDeclaration', 'FunctionExpression', and 'TSDeclareFunction'. You can add more contexts to the contexts option.

ej612 commented 3 months ago

Hi @brettz9, Wow, fast as lightning, thanks! I wasn't aware of this option, would it make sense to make it the default for no-types to include interfaces? (Or perhapes even for all of the rules, I'm not an expert).

brettz9 commented 3 months ago

The rules have long been designed this way—I don't know if it was chosen for performance reasons or what, but it would seem to me to make sense to be on by default in more places or as someone requested, to have a config which enables all of the stricter settings (see #701). I'm frankly just not as inclined to work on doing this myself, but a PR would be welcome.

brettz9 commented 3 months ago

I think we can discuss any further concerns at #701 , so closing for now. Feel free to comment further there as needed.

ej612 commented 3 months ago

Is this really the same issue as #701? It would seem that over there, we're talking about creating one or multiple configs with all rules turned on, whereas here, we're not talking about enabling or disabling rules, but rather broadening the scope on which these rules apply. Or are you saying that it wouldn't work to make no-types apply to TS interfaces if the environment is plain JS? Wouldn't it simply be ignored? If we can't include TS interfaces for all environments, should we change the defaults for the recommended-typescript-error config to include interfaces?

brettz9 commented 3 months ago

Is this really the same issue as #701? It would seem that over there, we're talking about creating one or multiple configs with all rules turned on, whereas here, we're not talking about enabling or disabling rules, but rather broadening the scope on which these rules apply.

Part of #701 would be tweaking the rules with specific options (e.g., enabling more contexts).

Or are you saying that it wouldn't work to make no-types apply to TS interfaces if the environment is plain JS? Wouldn't it simply be ignored?

Yes, it'd simply be ignored. I think no-types is a candidate for just checking all environments, but for some rules, projects might not be interested in enforcing say perfect stylistic structure everywhere.

If we can't include TS interfaces for all environments, should we change the defaults for the recommended-typescript-error config to include interfaces?

I think we can include TS interfaces for all environments.

I'll reopen, as it's fine to just track this for no-types, especially as I think it makes sense to apply it everywhere.

github-actions[bot] commented 3 months ago

:tada: This issue has been resolved in version 48.4.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

brettz9 commented 3 months ago

I've added handling by default for the case you mentioned. Let me know if others come up.

ej612 commented 3 months ago

Wow faster than the flash, thanks heaps!!

ej612 commented 3 months ago

Hi @brettz9,

It looks like check-param-names has the same problem:

export class A {
    /**
     * @param paramA Something something
     */
    public methodA(_paramA: string): void {
        //
    }
};

The rule complains as expected: image

But if it's an interface:

export interface B {
    /**
     * @param paramA Something something
     */
    methodB(paramB: string): void
};

The rule doesn't warn. I only caught this because typedoc complained.

github-actions[bot] commented 3 months ago

:tada: This issue has been resolved in version 48.5.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

ej612 commented 3 months ago

Thanks so much for your reactivity! I updated to the new version and now I'm seeing the following:

interface A {
    /**
     * @param params Values for the placeholders
     */
    getText(...params: string[]): string
}

All is well here. However, if I add a parameter to the function:

interface A {
    /**
     * @param params Values for the placeholders
     */
    getText(key: string, ...params: string[]): string
}

I now get the following error: image

Is this expected behavior? Thank you very much in advance!

Edit: If I add an @param entry for key, all is well again.

brettz9 commented 3 months ago

Dupe of #1225 .