eslint-plugin-jsdoc icon indicating copy to clipboard operation
eslint-plugin-jsdoc copied to clipboard

check-param-names ignore "@param path declaration appears before any real parameter" warning

Open JJJGGGG opened this issue 4 years ago • 7 comments

Motivation

When using react, documenting props is tedious because of that we should put the @param props every time or else not document the props in the JsDoc. This is worse if we have the rule, because then we are forced to describe what the props parameter is each time, given that the description is obvious.

When using react, we need an easy way to document component props. This is easy thanks to the destructuring property in require-param-names. The problem is that if the require-param-description rule is active, we should put a description for the props param, even if it is obvious.

Current behavior

This code:

/**
 * Description
 *
 * @param props.b something
 */
function A(props: { b: string }) {
...
}

with these settings:

    "jsdoc/require-param": ["warn", {"checkDestructuredRoots": false],
    "jsdoc/require-param-description": ["warn"],
    "jsdoc/check-param-names": ["warn", {"checkDestructured": true, "allowExtraTrailingParamDocs": false}],

Gives this warning:

@param path declaration ("props.to") appears before any real parameter

Desired behavior

Having the @props parameter description to be optional if it is a jsx component.

Otherwise, having the @props param to not be required if it is a jsx component.

Alternatives considered

None.


Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

JJJGGGG avatar Jun 10 '21 02:06 JJJGGGG

Hi,

I agree with that enhancement need.

Could these kind of comments be valid without the root parameter being inserted during linting:

/**
 * A module
 *
 * @param {object} reader
 * @return {JSX.Element}
 * @constructor
 */
const Module = ({reader}) => <div>{reader.text}</div>;

Should the following options not do that?

"jsdoc/require-param": [
	2,
	{
		"checkDestructuredRoots": false,
		"checkDestructured": true
	}
]

ccambournac avatar Jan 25 '22 09:01 ccambournac

@ccambournac : Yes, your options will work.

Another workaround which will work even with checkDestructuredRoots: true is to avoid using the object type and instead use something like the TypeScript object shorthand to document the object containing the reader property:

/**
 * A module
 *
 * @param {{reader: Reader}} obj
 * @return {JSX.Element}
 * @constructor
 */

And if that is still too much extra text for you, you could use a reusable typedef:

/**
 * @typedef {object} ReaderObj
 * @property reader
 */

/**
 * A module
 *
 * @param {ReaderObj} obj
 * @return {JSX.Element}
 * @constructor
 */

But as far as the OP, having @param props.b something without a definition for props is not standard JSDoc per https://jsdoc.app/tags-param.html#parameters-with-properties , so I think check-param-names (which is the actual rule complaining here, not require-param or require-param-name) should not be changed. However, we could indeed detect JSX (through @returns, a return of JSX in the code, or perhaps just when a single props is found) to prevent a description being required by require-param-description.

brettz9 avatar Jan 25 '22 09:01 brettz9

Hi @brettz9 and thanks for the reply.

I indeed posted a bit too quick (playing around with this new-to-me eslint plugin) and, as you point out, there are other rules involved. Like check-param-names here.

However, does my point not still hold? I mean if I want that destructuring of input parameters be considered as a list of parameters, not taking into account the root object, which is quite a common functional pattern, should not the checkDestructuredRoots option of require-param set to false apply implicitly to related rules like require-param-description, require-param-name, require-param-type. Actually, this is what I have with my above rules:

"jsdoc/require-param": [
	2,
	{
		"checkDestructuredRoots": false,
		"checkDestructured": true,
		"checkRestProperty": true
	}
],
"jsdoc/require-param-description": 2,
"jsdoc/require-param-name": 2,
"jsdoc/require-param-type": 2,
"jsdoc/check-param-names": [
	2,
	{
		"checkDestructured": false
	}
],

image

Maybe a check-param-descriptions, like check-param-names, with a checkDestructuredRoots option would be beneficial.

Or am I missing something? Maybe related to standard JSDoc preventing the plugin from doing that.

Anyway, thank you for the workaround suggestions. I would however like to stick as much as possible to an existing codebase when adding theses rules for my coworkers.

ccambournac avatar Jan 29 '22 14:01 ccambournac

However, does my point not still hold? I mean if I want that destructuring of input parameters be considered as a list of parameters, not taking into account the root object, which is quite a common functional pattern, should not the checkDestructuredRoots option of require-param set to false apply implicitly to related rules like require-param-description, require-param-name, require-param-type. Actually,addin this is what I have with my above rules:

ESLint options are not shared between rules. We would have to change checkDestructuredRoots to a setting for other rules to gain access.

check-param-names should probably have a checkDestructuredRoots option (or access the same setting) so long as we have one for require-param, but as for the other rules, projects which do want to require a name, type, and description may wish for them to be documented, consistent with other params, so I do not think we should automatically disable such checking.

That being said, I can see a use case for allowing and even requiring the root and yet disabling the need for a type or description.

Perhaps this could be resolved by a separate global setting available to rules such as exemptDestructuredRootsFromChecks. Only when such a setting is present, the types and descriptions would be exempted.

I think this makes more sense than specifically React-aware code. But it does come at the cost of adding complexity to the other rules which would now need to determine whether a param had destructured children or not. But probably worth it. We could create a reusable utility for this purpose.

Note, however, that this is not on my priority list, though PRs would be welcome.

Or am I missing something? Maybe related to standard JSDoc preventing the plugin from doing that.

Nothing preventing our rules from doing such detection except for the concern of balancing complexity. Standard JSDoc doesn't really insist that params have a type or description or arguably even a name: https://jsdoc.app/tags-param.html

On the topic of standard JSDoc though, I'd be curious to know how well other tools that people are using work without destructuring roots, e.g., IDE tooltips or documentation building.

brettz9 avatar Jan 31 '22 01:01 brettz9

@ccambournac : It should be helpful though if you could file a new issue for the other rules like require-param-type not complaining based on a new setting exemptDestructuredRootsFromChecks. I think this issue should be concerned with allowing check-param-names to be tolerant of missing destructured roots (though noting that we don't want to bend over backwards elsewhere in our code to support this non-standard practice).

brettz9 avatar Jan 31 '22 01:01 brettz9

Hi @brettz9 and thank you for your enlightening comments!

ESLint options are not shared between rules. We would have to change checkDestructuredRoots to a setting for other rules to gain access.

That makes sense :)

check-param-names should probably have a checkDestructuredRoots option (or access the same setting) so long as we have one for require-param, but as for the other rules, projects which do want to require a name, type, and description may wish for them to be documented, consistent with other params, so I do not think we should automatically disable such checking.

I agree. Maybe add the checkDestructuredRoots option support to the require-param-description rule too as it is the rule that raises the error in my latter example above or create a check-param-descriptions rule.

Note, however, that this is not on my priority list, though PRs would be welcome.

I understand.

Nothing preventing our rules from doing such detection except for the concern of balancing complexity. Standard JSDoc doesn't really insist that params have a type or description or arguably even a name: https://jsdoc.app/tags-param.html

Which, to my opinion, favors a fine-grained configuration of related rules.

As you requested, I added below how JSDoc can be automatically injected by typing /** + Enter in my IDE (PhpStorm). Last frame shows the result after manually inserting missing comments (description, param types):

jsdoc_idea

Finally, I will file a new issue as you suggest.

ccambournac avatar Feb 01 '22 09:02 ccambournac

check-param-names should probably have a checkDestructuredRoots option (or access the same setting) so long as we have one for require-param, but as for the other rules, projects which do want to require a name, type, and description may wish for them to be documented, consistent with other params, so I do not think we should automatically disable such checking.

I agree. Maybe add the checkDestructuredRoots option support to the require-param-description rule too as it is the rule that raises the error in my latter example above or create a check-param-descriptions rule.

I'd prefer we use a new setting exemptDestructuredRootsFromChecks since a project might wish to check that destructured roots but avoid the need for a description. Such users could maintain one global setting checkDestructuredRoots: true and add another global setting exemptDestructuredRootsFromChecks: true.

Nothing preventing our rules from doing such detection except for the concern of balancing complexity. Standard JSDoc doesn't really insist that params have a type or description or arguably even a name: https://jsdoc.app/tags-param.html

Which, to my opinion, favors a fine-grained configuration of related rules.

Yup.

As you requested, I added below how JSDoc can be automatically injected by typing /** + Enter in my IDE (PhpStorm). Last frame shows the result after manually inserting missing comments (description, param types):

jsdoc_idea

I see, thanks. That example is really more of a real bug, imo, as it doesn't follow jsdoc even basically as far as objects. Hopefully other IDEs follow this more faithfully.

Finally, I will file a new issue as you suggest.

Please link to it here once you have done so.

brettz9 avatar Feb 03 '22 01:02 brettz9

While the idea for an exemptDestructuredRootsFromChecks setting still makes sense, as far as adding checkDestructuredRoots to check-param-names, there is no need for this as checkDestructured already provides the functionality of avoiding stepping into the children, and the presence of any documented entry will be detected as a root (as it should by standard JSDoc). Any items not present in the docs are not checked by the rule.

brettz9 avatar Oct 27 '22 17:10 brettz9

One workaround is to use the contexts option.

For example, the following will avoid reporting (e.g., for require-param-description):

        {
          contexts: [
            {
              comment: 'JsdocBlock:has(JsdocTag:not([name=props]))',
              context: 'FunctionDeclaration',
            },
          ],
        },

brettz9 avatar Oct 27 '22 17:10 brettz9

I've submitted #929 which should address the remaining items that I feel should be supported (including the issue I originally asked be submitted as a separate issue). With the PR, one will be able to avoid (or auto-add) a type and description for such root objects (and require-param can already add missing root params + names). But I don't think we should be allowing for dropping roots as per the OP given it is expected in JSDoc. The require-param rule has this, I believe, just to tolerate terminal undocumented constructs not to skip over roots entirely.

brettz9 avatar Oct 27 '22 23:10 brettz9

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

The release is available on:

Your semantic-release bot :package::rocket:

gajus avatar Oct 29 '22 23:10 gajus