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

docs: add support for multiple signatures #592

Closed
ST-DDT opened this issue Mar 2, 2022 · 7 comments · Fixed by #596
Closed

docs: add support for multiple signatures #592

ST-DDT opened this issue Mar 2, 2022 · 7 comments · Fixed by #596
Assignees
Labels
c: docs Improvements or additions to documentation c: feature Request for new feature

Comments

@ST-DDT
Copy link
Member

ST-DDT commented Mar 2, 2022

Currently, we only show the first signature of a method.
This results in incomplete information being see below for an example.

I considered these options:

  • Switch to the last at that is usually the most accurate one
  • Display them all. The only questions is: How?

This involves the following questions:

  • Which values should we show in the parameter table and the return statement
  • Should we include them in the generated example section somehow?

This issue is here to discuss this.


Here is the only method that does use multiple signatures at this point in time:

faker/src/random.ts

Lines 174 to 185 in 60d3cc5

objectElement<T extends Record<string, unknown>, K extends keyof T>(
object: T,
field: 'key'
): K;
objectElement<T extends Record<string, unknown>, K extends keyof T>(
object: T,
field?: unknown
): T[K];
objectElement<T extends Record<string, unknown>, K extends keyof T>(
object = { foo: 'bar', too: 'car' } as unknown as T,
field = 'value'
): K | T[K] {

And it does kind of show up wrong:
https://fakerjs.dev/api/random.html#objectElement

Originally posted by @ST-DDT in #574 (comment)

@ST-DDT ST-DDT added the c: docs Improvements or additions to documentation label Mar 2, 2022
@ST-DDT ST-DDT moved this to Todo in Faker Roadmap Mar 2, 2022
@ST-DDT ST-DDT self-assigned this Mar 3, 2022
@ST-DDT ST-DDT added the c: feature Request for new feature label Mar 3, 2022
@Shinigami92
Copy link
Member

For now, get with the last one.
Sidenote: "Overloading" is only a concept of TypeScript but not JavaScript.

@Shinigami92
Copy link
Member

This PR is here to prevent me accidentally breaking the docs generation, not fixing issues that it currently has.

But this is not a PR?

@ST-DDT
Copy link
Member Author

ST-DDT commented Mar 3, 2022

This PR is here to prevent me accidentally breaking the docs generation, not fixing issues that it currently has.

But this is not a PR?

Yeah, I forgot to remove that line when opening this issue.

@xDivisionByZerox
Copy link
Member

Isn't this kinda obsolete/low priority since this will be deprecated in v6.2 by #503?

@ST-DDT
Copy link
Member Author

ST-DDT commented Mar 4, 2022

Well, the methods with formats might benefit from multiple signatures e.g. by limiting the return type to the exact match.
Not sure how many method we have with these though.

faker/src/time.ts

Lines 24 to 26 in a205c07

recent(
format: LiteralUnion<'abbr' | 'date' | 'wide' | 'unix'> = 'unix'
): string | number | Date {

@ST-DDT
Copy link
Member Author

ST-DDT commented Mar 4, 2022

It looks like it isn't that simple.
Typedoc does not detect all/the last signature and it does only add the comments to the first one.
I will try to debug that.

@Shinigami92
Copy link
Member

It looks like it isn't that simple. Typedoc does not detect all/the last signature and it does only add the comments to the first one. I will try to debug that.

So we need to open a PR at TypeDoc?

Repository owner moved this from Todo to Done in Faker Roadmap Mar 7, 2022
@ST-DDT ST-DDT removed this from Faker Roadmap Nov 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: docs Improvements or additions to documentation c: feature Request for new feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants