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

Standardize function parameters and defaults #1349

Closed
ST-DDT opened this issue Sep 6, 2022 · 9 comments · Fixed by #1784, #1804, #1805 or #1845
Closed

Standardize function parameters and defaults #1349

ST-DDT opened this issue Sep 6, 2022 · 9 comments · Fixed by #1784, #1804, #1805 or #1845
Assignees
Labels
breaking change Cannot be merged when next version is not a major release c: feature Request for new feature 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

Comments

@ST-DDT
Copy link
Member

ST-DDT commented Sep 6, 2022

Clear and concise description of the problem

Most methods have various ways to specify parameters and have different defaults.

Suggested solution

All methods should be checked that they have:

  • consistent parameter definitions
  • consistent parameter names
  • consistent parameter defaults

The consistency should span across all modules.

Alternative

No response

Additional context

No response

@ST-DDT ST-DDT added c: feature Request for new feature s: on hold Blocked by something or frozen to avoid conflicts p: 1-normal Nothing urgent c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs breaking change Cannot be merged when next version is not a major release labels Sep 6, 2022
@ST-DDT ST-DDT added this to the v8.0 - Module Re-Shuffling milestone Sep 6, 2022
@ST-DDT
Copy link
Member Author

ST-DDT commented Sep 6, 2022

Blocked by #1346

@ST-DDT
Copy link
Member Author

ST-DDT commented Oct 18, 2022

@Shinigami92
Copy link
Member

Shinigami92 commented Nov 4, 2022

@xDivisionByZerox
Copy link
Member

Is the LoremModule already considered "standardized"?
All functions already have an object argument, but with a more descriptive name than our standard "options".
For Example:

words(
wordCount:
| number
| {
/**
* The minimum number of words to generate.
*/
min: number;
/**
* The maximum number of words to generate.
*/
max: number;
} = 3
): string {

I'm not sure what to do here.

@matthewmayer
Copy link
Contributor

I was going to add a section to the migration guide about these changes, but to clarify is it intended that the non-options-object parameters will be deprecated in v8 and removed in v9? Or will both versions be supported in v8+?

@xDivisionByZerox
Copy link
Member

xDivisionByZerox commented Feb 19, 2023

TLDR;

So yes, some of the signatures will be deprecated in v8 and removed in v9. So they are usable for v8.x. But most of them simply get standardized into an options object and can be used even after v9.


[...] to clarify is it intended that the non-options-object parameters will be deprecated in v8 and removed in v9

No, not entirely. Primitive arguments are often desired for configuring the parameter which is most likely to be used the most by users. For example:

faker.number.int(10); // number between 0 and 10
faker.number.int({ max: 10 }); // number between 0 and 10
faker.number.int({ max: 10, min: 10 }); // number between 0 and 10

As you can see, all functions do exactly the same, but the biggest use-case for int is most likely to get a number between 0 and x. It would hurt the DX to require an options object altho you only want to change the max option.

The downside of primitives is that they are hard to maintain. You can extend signatures by always adding an additional parameter at the end (example faker.number.int(min, max, steps, format, foo, bar)). But as soon as you need to change or remove one of them it gets awkward. Thats why we decided that each function signature essentially should follow this pattern:

  1. Function has no arguments
  2. Function has object argument (typically named options)
  3. Function can have overload with options argument OR at most one primitive argument
  4. Exceptions may apply (helpers most likely)

@ST-DDT ST-DDT added the s: needs decision Needs team/maintainer decision label Feb 19, 2023
@github-project-automation github-project-automation bot moved this from In Progress to Done in Faker Roadmap Mar 14, 2023
@ST-DDT
Copy link
Member Author

ST-DDT commented Mar 16, 2023

Team Decision

We will rewrite the lorem methods to support options.
The range object support that was added in the alpha will be removed and will only be available from inside the options object.

@ST-DDT ST-DDT added s: accepted Accepted feature / Confirmed bug and removed s: needs decision Needs team/maintainer decision labels Mar 16, 2023
@ST-DDT ST-DDT moved this from Done to In Progress in Faker Roadmap Mar 16, 2023
@Shinigami92
Copy link
Member

It seems we have tackled all needed changes. If we want to change something additionally, we should open issues/PRs specifically.

@github-project-automation github-project-automation bot moved this from In Progress to Done in Faker Roadmap May 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Cannot be merged when next version is not a major release c: feature Request for new feature 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: Done
4 participants