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

[check-param-names] Support disabling destructuring #616

Closed
brawaru opened this issue Jul 28, 2020 · 5 comments · Fixed by #618
Closed

[check-param-names] Support disabling destructuring #616

brawaru opened this issue Jul 28, 2020 · 5 comments · Fixed by #618

Comments

@brawaru
Copy link

brawaru commented Jul 28, 2020

Motivation

When using TypeScript and React it's really simple to write your components and their props like that:

/**
 * Properties for the Greeting component
 */
interface IGreetingProps {
  /**
   * Who is greeting to
  */
  target: string;
}

/**
 * @returns Greeting component
 */
function Greeting({ target }: IGreetingProps) {
  return <>Hello, {target}!</>;
}

/**
 * @returns App component
 */
export default function App() {
  return (
    <div>
      <Greeting target='world' />
    </div>
  );
}

Current behavior

Unfortunately, in some of recent (not that I missed about five months of releases) versions defaults now ask you:

  • To document root1 (param1 according to TypeScript) parameter (that's okay, I can do that even that it is never displayed anywhere?)
  • To document all properties of that root1, so in our case param1.target (that's not okay and is useless duplication for TypeScript)

Desired behavior

Be able to either (or both):

  • Disable name checks for destructured parameters (require only root1/param1 to be documented)
  • Disable root1 parameters entirely (require only named parameters to be documented and skip automatic ones)

Alternatives considered

Someone seemed to reported something the same (#530) and it was marked as fixed. It was related to require-param and fix added checkDestructured option, but this rule does not affect check-param-names.

brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue Jul 28, 2020
…sabling of destructured checking; fixes part of gajus#616

Also improves display of errors when destructured properties are present
@brettz9
Copy link
Collaborator

brettz9 commented Jul 28, 2020

I've filed #617 which should address your first desired behavior.

For the second desired behavior, we'd need to add a new option--any suggested names for the option? Maybe checkDestructuredRoots?

This latter option should I think only be applied to require-param though, as check-param-names only iterates the present param docs, so if a destructured object occurs later in the signature, and there is no @param for it, there will be no reporting of a missing root.

I guess some might want checkDestructuredRoots: false to allow the following for check-param-names when a destructured param occurs earlier in the signature and declares no root:

          /**
           * @param obj.foo
           * @param root
           */
          function quux ({foo}, root) {

          }

...but this would not be standard jsdoc, as jsdoc requires the root have its own declaration.

@brettz9
Copy link
Collaborator

brettz9 commented Jul 28, 2020

Btw, if you want the style param1 (how you indicate TypeScript reports it) to be reported in require-param in place of root0 as is the default, you can add these properties to your options object:

{
  autoIncrementBase: 1,
  unnamedRootBase: 'param'
}

@brawaru
Copy link
Author

brawaru commented Jul 28, 2020

I tested #617 and it seems to resolve my problem 🙌 Thanks!

And you're right, the latter option is out of check-param-names scope (I missed that, my bad). I see now the problem with mixed named and unnamed (destructured) ‘roots’, I didn't think of that, that's problematic. I just thought of that only TypeScript React exception, where the only parameter passed is the props, that you can immediately destructure if you only need a few keys:

function Component(props: ComponentProps) { }
function Component({ children }: ComponentProps) { }

It's just… there is no sense for you to document destructured props parameter (“Properties for the component”), it will never show up in intellisense suggestions and not very useful in documentation and I believe it will be still picked up by documentation generators for TypeScript even if you didn't declare it in JSDocs. So I think, maybe you can name this exception as checkRootOnly or somewhat like this, so if there are other named parameters — function func({ foo }, bar), then it would require you to document both, but if there are none function Comp({ children }) then it will not require you to comment it? That's very specific, though.

@brettz9
Copy link
Collaborator

brettz9 commented Jul 28, 2020

I think it makes sense for us to keep checkDestructured: false as it has been in use already, and that already behaves to check the root only in cases such as bar in function func({ foo }, bar) {}).

I think if we allow checkDestructuredRoots: false (or checkRoots: false) to disable root checking in require-param, when used with checkDestructured: false, there will be no reporting at all by require-param for these following you mention (and, as discussed, check-param-names won't complain when param tags are missing), while still reporting both roots for function func({ foo }, bar) {} (since we can simply document that checkDestructuredRoots has no effect when a subsequent non-destructured param exists, as there needs to be a placeholder):

function Component(props: ComponentProps) { }
function Component({ children }: ComponentProps) { }

brettz9 added a commit that referenced this issue Jul 31, 2020
…sabling of destructured checking; fixes part of #616

Also improves display of errors when destructured properties are present
brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue Jul 31, 2020
@gajus
Copy link
Owner

gajus commented Aug 4, 2020

🎉 This issue has been resolved in version 30.2.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