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

require-param reports false negative since v17.0.1 #443

Closed
StephanBijzitter opened this issue Dec 3, 2019 · 9 comments
Closed

require-param reports false negative since v17.0.1 #443

StephanBijzitter opened this issue Dec 3, 2019 · 9 comments

Comments

@StephanBijzitter
Copy link

Decided to update from v15 to v18 and one of the rules started acting up, so I tried a few different versions and found that v17.0.1 is the culprit.

The following piece of code used to pass the require-param check:

     * @param args Arguments compiled and provided by <redacted>.
     * @param args.options The options as provided by the user, or an empty object if not provided.
     * @param defaultOptions The default options as provided by the plugin, or an empty object.
     */
    public constructor({options, client}: {
        options: O;
        client: Redacted;
    }, defaultOptions: D) {
        this.client = client;
        this.options = merge(defaultOptions, options) as O & D;
        this.logger = signale.scope(`plugin:${this.constructor.name.replace(/redacted/gui, '').toLowerCase()}`);
    }

But now, it complains: Missing JSDoc @param "defaultOptions" declaration.
The automatic fixer wasn't of much help 😅 :

     * @param args Arguments compiled and provided by DullahanClient.
     * @param args.options The options as provided by the user, or an empty object if not provided.
     * @param defaultOptions The default options as provided by the plugin, or an empty object.
     * @param defaultOptions
     * @param defaultOptions
     * @param defaultOptions
     * @param defaultOptions
     * @param defaultOptions
     * @param defaultOptions
@brettz9
Copy link
Collaborator

brettz9 commented Dec 3, 2019

I have added the following code to our testing framework and found no issues reported:

    {
      code: `
      class Quux {
        /**
         * @param args Arguments compiled and provided by <redacted>.
         * @param args.options The options as provided by the user, or an empty object if not provided.
         * @param defaultOptions The default options as provided by the plugin, or an empty object.
         */
        public constructor({options, client}: {
            options: O;
            client: Redacted;
        }, defaultOptions: D) {
            this.client = client;
            this.options = merge(defaultOptions, options) as O & D;
            this.logger = signale.scope(\`plugin:\${this.constructor.name.replace(/redacted/gui, '').toLowerCase()}\`);
        }
      }
      `,
      parser: require.resolve('@typescript-eslint/parser'),
    },

@StephanBijzitter
Copy link
Author

Weird, I do get issues. Maybe it's triggered because of another part of the documentation block?

I've stripped the file from a lot of content (and replaced all words I cannot share in public with my own name) and created a gist: https://gist.github.com/StephanBijzitter/6cee238bf73042ca37666ed2b5384df1

Running ESLint against this file now gives me the following errors:

v18.4.1, v17.0.1 (1 jsdoc error)

   3:5   error  Missing JSDoc @param "defaultOptions" declaration  jsdoc/require-param
  36:5   error  Useless constructor                                no-useless-constructor
  36:25  error  'options' is defined but never used                @typescript-eslint/no-unused-vars
  36:34  error  'client' is defined but never used                 @typescript-eslint/no-unused-vars
  39:8   error  'defaultOptions' is defined but never used         @typescript-eslint/no-unused-vars
  39:27  error  Unexpected empty constructor                       no-empty-function

v17.0.0 (0 jsdoc error)

  36:5   error  Useless constructor                         no-useless-constructor
  36:25  error  'options' is defined but never used         @typescript-eslint/no-unused-vars
  36:34  error  'client' is defined but never used          @typescript-eslint/no-unused-vars
  39:8   error  'defaultOptions' is defined but never used  @typescript-eslint/no-unused-vars
  39:27  error  Unexpected empty constructor                no-empty-function

@brettz9
Copy link
Collaborator

brettz9 commented Dec 3, 2019

Ah yes, 17.0.1 added an update from comment-parser which avoided treating ampersand-prefixed items within @example (like decorators) as being actual new jsdoc tags, so it makes sense the issue has an @example.

Am in the middle of something now, but you might test on runkit to see how comment-parser is currently parsing your code: https://npm.runkit.com/comment-parser . Will try to take a look later.

@StephanBijzitter
Copy link
Author

Cool, I haven't used comment-parser before (or runkit, that's quite awesome)!

I see tag: param, name: defaultOptions in the output and it also has the description and whether it's optional or not. So as far as I can tell, that works

@brettz9
Copy link
Collaborator

brettz9 commented Dec 4, 2019

We should look into why comment-parser is not handling this properly, but as a workaround (and what I'm guessing you wanted to do anyways), change this line:

*```typescript

to add a space after the asterisk, i.e., to this:

* ```typescript

Then comment-parser properly parses, and we treat it as valid.

@brettz9
Copy link
Collaborator

brettz9 commented Dec 4, 2019

The comment-parser has been merged, and so we are just awaiting an npm release to update. But again, the workaround is available now if you like.

@brettz9 brettz9 added the bug label Dec 4, 2019
@brettz9 brettz9 closed this as completed in dd5f015 Dec 4, 2019
@gajus
Copy link
Owner

gajus commented Dec 4, 2019

🎉 This issue has been resolved in version 18.4.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@gajus gajus added the released label Dec 4, 2019
@brettz9
Copy link
Collaborator

brettz9 commented Dec 4, 2019

This should now be fully fixed without need for the workaround. Thanks for the report and info!

@StephanBijzitter
Copy link
Author

StephanBijzitter commented Dec 4, 2019

@brettz9 Tested 18.4.2 with the workaround*, works.
Tested 18.4.3 without the workaround, works.

  • = You're right, I didn't even notice there was no space between the star and backtick in that location! I added the space and left it in there, it looks cleaner anyway.

Thanks for this really quick fix!

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

No branches or pull requests

3 participants