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

Clearer documentation for fakerBASE #2023

Closed
matthewmayer opened this issue Apr 4, 2023 · 6 comments · Fixed by #3193
Closed

Clearer documentation for fakerBASE #2023

matthewmayer opened this issue Apr 4, 2023 · 6 comments · Fixed by #3193
Assignees
Labels
c: docs Improvements or additions to documentation c: infra Changes to our infrastructure or project setup c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Milestone

Comments

@matthewmayer
Copy link
Contributor

matthewmayer commented Apr 4, 2023

Clear and concise description of the problem

I don't see the value in exposing fakerBASE (or having the base key in allFakers) implemented in #1748 .

import {base} from "@faker-js/faker" is sufficient if you want to create a Faker instance based on base. fakerBASE by itself is not very useful, might cause confusion for new users, and will need special cases for things like #1984

Suggested solution

Don't expose fakerBASE

Alternative

Clearer documentation

Additional context

No response

@matthewmayer matthewmayer added s: pending triage Pending Triage s: needs decision Needs team/maintainer decision labels Apr 4, 2023
@matthewmayer matthewmayer moved this to Todo in Faker Roadmap Apr 4, 2023
@Shinigami92
Copy link
Member

Not exposing fakerBASE is not an option from my side

So if no other maintainer is of another opinion, we need to got the alternative route

@ST-DDT ST-DDT added p: 1-normal Nothing urgent c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs c: infra Changes to our infrastructure or project setup and removed s: pending triage Pending Triage labels Apr 4, 2023
@ST-DDT
Copy link
Member

ST-DDT commented Apr 4, 2023

Not exposing fakerBASE is not an option from my side

Could you explain why it is not an option, please? That adds more weight to your argument.


and will need special cases for things like #1984

IMO only by the same amount as custom locales would. E.g. use the "Partial"<MetadataDefinitions> instead of the "Full"MetadataDefinitions as the other locales would require.


fakerBASE by itself is not very useful

IMO fakerBASE adds a lightweight thing to import if you don't need localized data.
But that could also be covered by a fakerNONE (or a custom) instance, that doesn't have any locales at all.
That we should probably discuss adding as well.


might cause confusion for new users

IMO we should generate jsdocs for all our pre-built faker istances and probably the locales.
For the base locale this might actually require some custom text, but IMO that is acceptable.
(The same might be true for locales that are a variant of another locale)

@matthewmayer
Copy link
Contributor Author

I think some of the confusion is that on the surface, fakerBASE only appears to contain quite obscure data like faker.database.collation and faker.color.space. You are saying that the value in it is not for that, but for completely non-localized methods like if you just want things like faker.number.int() and faker.helpers.arrayElement()?

@matthewmayer matthewmayer changed the title Don't expose fakerBASE Clearer documentation for fakerBASE Apr 4, 2023
@ST-DDT ST-DDT added the c: docs Improvements or additions to documentation label Apr 4, 2023
@Shinigami92
Copy link
Member

Could you explain why it is not an option, please? That adds more weight to your argument.

I'm sorry, I was more or less a bit stressed.

So here are some reasons:

  1. I don't want/like to maintain extra stuff for the base locale for that (like defining branches in the script folder)
  2. the base locale and the base instance is theoretically not different then the other locales, it's just the base/ground whatever
  3. calling "it's to complex for unexperienced users" is IMO a really bad reason
    either the docs should be improved (as you suggested in the alternative section) or the "unexperienced users" should ignore it or get experienced with it 🤷

@matthewmayer
Copy link
Contributor Author

matthewmayer commented Apr 5, 2023

Changed the title to reflect improving the documentation better instead.

One thing I've been thinking is that it's hard looking at the docs to tell which methods are reliant on the localizable definitions (like faker.person.firstName() or faker.internet.email()) and which are not (like faker.number.int() or faker.helpers.arrayElement()). Perhaps there could be a standardized way to indicate this, like "Localizable." at the end of the description, or an icon.

@ST-DDT
Copy link
Member

ST-DDT commented Apr 20, 2023

Team Decision

  • We want to generate/add descriptions for fakerBASE and the other faker and locale instances.
    • fakerBASE/base (localeBASE) will use a custom text
    • fakerX/localeX will use a generated text from the metadata

@ST-DDT ST-DDT added s: accepted Accepted feature / Confirmed bug and removed s: needs decision Needs team/maintainer decision labels Apr 20, 2023
@ST-DDT ST-DDT self-assigned this Oct 18, 2024
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: infra Changes to our infrastructure or project setup c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Status: Todo
Development

Successfully merging a pull request may close this issue.

3 participants