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

Make check-param-names to allow documenting non-defined parameters #450

Closed
yvele opened this issue Dec 23, 2019 · 5 comments · Fixed by #454
Closed

Make check-param-names to allow documenting non-defined parameters #450

yvele opened this issue Dec 23, 2019 · 5 comments · Fixed by #454

Comments

@yvele
Copy link

yvele commented Dec 23, 2019

I've migrated from the native ESLint JSDoc rules and so far everything went easy to implement, thank you so much for this project 😍

I have use cases where sometimes I need to add JSDoc on a function parameter that is not explicitly present on the function signature.

First use case is when I implement a "interface". On the concrete function (that implements the "interface"), I want to add JSDoc on ALL the parameter even though they are not consumed by the implementation.

/**
 * This function implements an "interface" that defines 2 parameters.
 *
 * @param {string} a
 * @param {string} b
 * @returns {string}
 */
function foo(a) {
  console.log(`I'm only using a but I still need to see b in the JSDoc`);
  return a;
}

But I got an error with the check-param-names rule

@param "b" does not match an existing function parameter

Is it possible to add an option to the check-param-names rule in order to allow non-defined function parameters? But of course only non-defined at THE END of the argument list.. Non-defined parameters that are not LAST will still make the rule to throw.

🤔

Note that the old native ESLint JSDoc rule was allowing adding JSDoc on non-defined last arguments https://eslint.org/docs/rules/valid-jsdoc

Edit: In my use cases the JSDoc is super important for the developers that reads, implements and maintains the code. JSDoc is not only important for consumers but also for implementers. 😉

PS: Also, what about the arguments use case? Let's say I'm having this:

/**
 * @param {string} a
 * @param {string} b
 * @returns {string}
 */
function foo() {
  return arguments[0];
}
@yvele yvele changed the title Make check-param-names to allow documenting non existing parameters Make check-param-names to allow documenting non-defined parameters Dec 23, 2019
@brettz9
Copy link
Collaborator

brettz9 commented Dec 23, 2019

I think this is a reasonable option to request, but as far as arguments, that seems to be generally discouraged in favor of rest params (e.g., ...args); e.g., see https://eslint.org/docs/rules/prefer-rest-params . (It doesn't even add to function length so function a (b, ...args) {} only has a length of 1.) So your second example could be implemented/documented as:

/**
 * @param {...string} args Non-signature arguments
 * @returns {string} Returns the first non-signature argument.
 */
function foo(...args) {
  return args[0];
}

Admittedly though, if you really needed to document args per argument, you couldn't use an array typedef as jsdoc didn't implement as @param {string[]} ...args.

@yvele
Copy link
Author

yvele commented Dec 23, 2019

I'm not using arguments in my code but opening this issue was making me thinking of this other use case (maybe other people have legacy code, or special use cases..).

My real use case is for unused arguments I still need to document in JSDoc 😉

@yvele
Copy link
Author

yvele commented Dec 23, 2019

I cannot use your "Non-signature workaround" as I still need to document the first arguments that are used.

Also the way we use JSDoc should not impact the code implementation at all.

@brettz9
Copy link
Collaborator

brettz9 commented Dec 23, 2019

Re: your real use case of unfinished code (or document-driven-development), sure, and that is why I said it was a reasonable option to request.

Re: the non-signature workaround, yes, that was the point of my caveat; it won't be able to handle distinct typing for each particular argument within the spread--which I think probably ought to be changed by jsdoc.

As far as not impacting code implementations, this is generally true (though the likes of our require-returns-check rule are meant to impact your implementation; if you are committed to documenting it as having a return and you don't, it expects the implementation to change).

However, whether rightly or wrongly, the current MDN page for arguments states:

Note: If you're writing ES6 compatible code, then rest parameters should be preferred.

Source: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/arguments

I don't think we want to spend as much effort making rules supporting use of deprecated features. But granted, arguments is pretty widespread in legacy code, so I understand an argument for better handling it even here, and it is of course still a part of the spec and implementations.

So I mention this more for the sake of a full discussion and for the possible edification of all users, rather than to knock down your whole proposal (which I also happen to like) or even negate the arguments aspect entirely.

brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue Dec 23, 2019
…o avoid reporting additional `@param`'s beyond actual function's arguments; fixes gajus#450
brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue Dec 23, 2019
…o avoid reporting additional `@param`'s beyond actual function's arguments; fixes gajus#450
brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue Dec 27, 2019
…o avoid reporting additional `@param`'s beyond actual function's arguments; fixes gajus#450
brettz9 added a commit that referenced this issue Dec 27, 2019
…o avoid reporting additional `@param`'s beyond actual function's arguments; fixes #450
@gajus
Copy link
Owner

gajus commented Dec 27, 2019

🎉 This issue has been resolved in version 18.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging a pull request may close this issue.

3 participants