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

exemptDestructuredRootsFromChecks setting for param type and description checks to be dropped (e.g., React props) #752

Closed
JJJGGGG opened this issue Jun 10, 2021 · 11 comments · Fixed by #929

Comments

@JJJGGGG
Copy link

JJJGGGG commented Jun 10, 2021

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.

@ccambournac
Copy link

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
	}
]

@brettz9
Copy link
Collaborator

brettz9 commented Jan 25, 2022

@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.

@ccambournac
Copy link

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.

@brettz9
Copy link
Collaborator

brettz9 commented Jan 31, 2022

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 brettz9 changed the title require-param-names ignore "@param path declaration appears before any real parameter" warning check-param-names ignore "@param path declaration appears before any real parameter" warning Jan 31, 2022
@brettz9
Copy link
Collaborator

brettz9 commented Jan 31, 2022

@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).

@ccambournac
Copy link

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.

@brettz9
Copy link
Collaborator

brettz9 commented Feb 3, 2022

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

brettz9 commented Oct 27, 2022

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 brettz9 changed the title check-param-names ignore "@param path declaration appears before any real parameter" warning exemptDestructuredRootsFromChecks setting for param type and description checks to be dropped (e.g., React props) Oct 27, 2022
@brettz9
Copy link
Collaborator

brettz9 commented Oct 27, 2022

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 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue Oct 27, 2022
…estructuredRootsFromChecks` setting; fixes gajus#752

Also:
- feat(`require-param-type`): add `setDefaultDestructuredRootType` and `defaultDestructuredRootType` options
- feat(`require-param-description`): add `setDefaultDestructuredRootDescription` and `defaultDestructuredRootDescription` options
@brettz9
Copy link
Collaborator

brettz9 commented Oct 27, 2022

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 added a commit that referenced this issue Oct 29, 2022
…estructuredRootsFromChecks` setting; fixes #752

Also:
- feat(`require-param-type`): add `setDefaultDestructuredRootType` and `defaultDestructuredRootType` options
- feat(`require-param-description`): add `setDefaultDestructuredRootDescription` and `defaultDestructuredRootDescription` options
@gajus
Copy link
Owner

gajus commented Oct 29, 2022

🎉 This issue has been resolved in version 39.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@gajus gajus added the released label Oct 29, 2022
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this issue Oct 31, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [eslint-plugin-jsdoc](https://github.com/gajus/eslint-plugin-jsdoc) | devDependencies | minor | [`39.3.25` -> `39.4.0`](https://renovatebot.com/diffs/npm/eslint-plugin-jsdoc/39.3.25/39.4.0) |

---

### Release Notes

<details>
<summary>gajus/eslint-plugin-jsdoc</summary>

### [`v39.4.0`](https://github.com/gajus/eslint-plugin-jsdoc/releases/tag/v39.4.0)

[Compare Source](gajus/eslint-plugin-jsdoc@v39.3.25...v39.4.0)

##### Features

-   **`require-param-type`, `require-param-description`:** add `exemptDestructuredRootsFromChecks` setting; fixes [#&#8203;752](gajus/eslint-plugin-jsdoc#752) ([da1c85f](gajus/eslint-plugin-jsdoc@da1c85f))

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC45LjEiLCJ1cGRhdGVkSW5WZXIiOiIzNC45LjEifQ==-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1617
Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
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.

4 participants