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

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

Closed
ej612 opened this issue Jun 24, 2024 · 13 comments · Fixed by #1250, #1251 or femdevs/femdev-website#18 · May be fixed by WontonSam/markdownlint#13 or WontonSam/markdownlint#22
Closed

Comments

@ej612
Copy link

ej612 commented Jun 24, 2024

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

  • Node version: 18
  • ESLint version 8.57.0
  • eslint-plugin-jsdoc version: 48.2.13
@brettz9
Copy link
Collaborator

brettz9 commented Jun 24, 2024

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

@ej612
Copy link
Author

ej612 commented Jun 24, 2024

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
Copy link
Collaborator

brettz9 commented Jun 24, 2024

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
Copy link
Collaborator

brettz9 commented Jun 24, 2024

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

@brettz9 brettz9 closed this as completed Jun 24, 2024
@ej612
Copy link
Author

ej612 commented Jun 24, 2024

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
Copy link
Collaborator

brettz9 commented Jun 24, 2024

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.

@brettz9 brettz9 reopened this Jun 24, 2024
brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue Jun 24, 2024
brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue Jun 24, 2024
brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue Jun 24, 2024
Copy link

🎉 This issue has been resolved in version 48.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@brettz9
Copy link
Collaborator

brettz9 commented Jun 24, 2024

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

@ej612
Copy link
Author

ej612 commented Jun 24, 2024

Wow faster than the flash, thanks heaps!!

@ej612
Copy link
Author

ej612 commented Jun 26, 2024

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.

Copy link

🎉 This issue has been resolved in version 48.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@ej612
Copy link
Author

ej612 commented Jun 26, 2024

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
Copy link
Collaborator

brettz9 commented Jun 26, 2024

Dupe of #1225 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment