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

validation.notDocumented doesn't correctly handle some cases #1898

Closed
thislooksfun opened this issue Mar 27, 2022 · 7 comments
Closed

validation.notDocumented doesn't correctly handle some cases #1898

thislooksfun opened this issue Mar 27, 2022 · 7 comments
Labels
bug Functionality does not match expectation

Comments

@thislooksfun
Copy link

Search terms

function type definitions, methods with computed names, generated types

Expected Behavior

Enabling validation.notDocumented should correctly report on:

  • function type definitions (type Foo = () => void)
  • methods with computed names:
    const name = "foo";
    class SomeClass {
      [name]() { return "huh"; }
    }
  • methods inside generated definitions used by excluded methods:
    class SomeClass {
      doSomething() {
        return {
          foo() {}
        }
      }
    }

Actual Behavior

All of the above cases falsely report that they are undocumented.

Steps to reproduce the bug

  1. Clone the main branch of https://github.com/thislooksfun/typedoc-repros
  2. Install deps and run npm run gendoc
  3. The docs should be generated without any warning. Instead, it prints:
Warning: FunctionType, defined at index.ts:2, does not have any documentation.
Warning: [iterator], defined at index.ts:7, does not have any documentation.
Warning: index, defined at index.ts:10, does not have any documentation.
Warning: next, defined at index.ts:12, does not have any documentation.
Warning: next, defined at index.ts:28, does not have any documentation.

Environment

  • Typedoc version: HEAD (98841f5)
  • TypeScript version: any
  • Node.js version: 16
  • OS: macOS
@thislooksfun thislooksfun added the bug Functionality does not match expectation label Mar 27, 2022
@thislooksfun
Copy link
Author

I also just noticed that for some incredibly strange reason it ignores @internal on every other function parameter (when passing requiredToBeDocumented contains Parameter, of course):

The following method definition:

class SomeClass {
  /** @internal */
  foobar(
    first: string,
    second: string,
    third: string,
    fourth: string,
    fifth: string,
    sixth: string,
    seventh: string,
    eighth: string
  ) {
    first;
    second;
    third;
    fourth;
    fifth;
    sixth;
    seventh;
    eighth;
  }
}

Fails with the error:

Warning: SomeClass.foobar.second, defined at src/reddit/subreddit/object.ts:429, does not have any documentation.
Warning: SomeClass.foobar.fourth, defined at src/reddit/subreddit/object.ts:431, does not have any documentation.
Warning: SomeClass.foobar.sixth, defined at src/reddit/subreddit/object.ts:433, does not have any documentation.
Warning: SomeClass.foobar.eighth, defined at src/reddit/subreddit/object.ts:435, does not have any documentation.

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Mar 27, 2022

Note to self, never accept a feature without a large battery of tests... I'm also being reminded of how much I dislike TypeDoc's use of reflections for object types :/

@thislooksfun
Copy link
Author

Heh, I know the feeling. I just had to revert a nice looking change in one of my projects that completely failed in production because I didn't test it enough.

Thankfully this particular bug is in a non-critical section, and only really matters if you have both validation.notDocumented and treatWarningsAsErrors turned on, so it could be much worse.

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Mar 27, 2022

on every other function parameter

This is an easy bug at least. First thing I checked was the issue. TypeDoc .traverse didn't clone lists when iterating, so if we remove from the list while iterating over it, we'll end up skipping every other item.

@thislooksfun
Copy link
Author

Yeah that'll do it! Removing things from arrays while iterating over them is so easy, it's one of my favorite things (massive /s). At least it was an easy fix. If my guess is correct the function types will be easy as well. Good luck on the other two though, they seem like they'll be a pain.

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Mar 27, 2022

Well, there's that fixed... hopefully this lasts longer than the other one before horrible bugs are discovered ;)

@thislooksfun
Copy link
Author

I can confirm that my project now passes without error when run against ea16a7b! I'm probably not using every possible reflected type, but I ran with requiredToBeDocumented set to everything other than "Project" and got 0 false matches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Functionality does not match expectation
Projects
None yet
Development

No branches or pull requests

2 participants