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

Refactor number.float parameter precision to fractionDigits #1596

Closed
Shinigami92 opened this issue Nov 24, 2022 · 12 comments · Fixed by #1855 or #2564
Closed

Refactor number.float parameter precision to fractionDigits #1596

Shinigami92 opened this issue Nov 24, 2022 · 12 comments · Fixed by #1855 or #2564
Assignees
Labels
c: feature Request for new feature deprecation A deprecation was made in the PR m: number Something is referring to the number module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Milestone

Comments

@Shinigami92
Copy link
Member

Shinigami92 commented Nov 24, 2022

Clear and concise description of the problem

We want to refactor the behavior of the precision parameter of number.float to be adjusted to Number#toFixed behavior.

This wont affect the legacy datatype.float method!

Suggested solution

  • precision -> fractionDigits
  • We then want to have a well defined developer experience and support steps like 0.4 and such
    So we might need an addition parameter like step(s) or precision and then talk about how this behaves

Alternative

No response

Additional context

Extracted from

@Shinigami92 Shinigami92 added c: feature Request for new feature s: on hold Blocked by something or frozen to avoid conflicts p: 1-normal Nothing urgent s: needs decision Needs team/maintainer decision m: number Something is referring to the number module labels Nov 24, 2022
@Shinigami92 Shinigami92 moved this to Todo in Faker Roadmap Nov 24, 2022
@Shinigami92
Copy link
Member Author

Team Decision:

We want support both parameters: fractionDigits and the second one
If they are incompatible throw an FakerError

@ST-DDT ST-DDT added s: accepted Accepted feature / Confirmed bug and removed s: needs decision Needs team/maintainer decision labels Nov 24, 2022
@Shinigami92 Shinigami92 removed the s: on hold Blocked by something or frozen to avoid conflicts label Dec 7, 2022
@xDivisionByZerox
Copy link
Member

I will take this.

@xDivisionByZerox xDivisionByZerox self-assigned this Jan 29, 2023
@ST-DDT
Copy link
Member

ST-DDT commented Jan 29, 2023

Please wait for #1675 to be merged.

@xDivisionByZerox xDivisionByZerox added the s: on hold Blocked by something or frozen to avoid conflicts label Jan 29, 2023
@xDivisionByZerox xDivisionByZerox removed the s: on hold Blocked by something or frozen to avoid conflicts label Feb 16, 2023
@ST-DDT
Copy link
Member

ST-DDT commented Feb 18, 2023

Continuation of the discussion that came up in #1855 (comment)

@matthewmayer suggested adding a new method for stepped values to separate it from float.

@ST-DDT
Copy link
Member

ST-DDT commented Feb 18, 2023

@matthewmayer Do you have a suggestion for a method name for the new step method?

Maybe faker.number.stepped?

@ST-DDT
Copy link
Member

ST-DDT commented Feb 23, 2023

Team decision

The proposal to move the precision/step logic into a different method has been rejected.
We will keep this precision/step option in the float method's single signature and document that the parameters are mutually exclusive.

@ST-DDT
Copy link
Member

ST-DDT commented Apr 13, 2023

Team Decision

We don't need this for v8.0.
(We will likely immediately start with v9 after v8.0)

@ST-DDT
Copy link
Member

ST-DDT commented May 30, 2023

Related issue: #2186

@ST-DDT ST-DDT removed this from the v8 - This Major Version milestone Sep 21, 2023
@ST-DDT ST-DDT added this to the vAnytime milestone Sep 21, 2023
@ST-DDT ST-DDT added the deprecation A deprecation was made in the PR label Oct 10, 2023
@ST-DDT ST-DDT modified the milestones: vAnytime, v8.x Oct 10, 2023
@matthewmayer
Copy link
Contributor

So basically

  • this would add a new parameter fractionDigits as an alternative to precision
  • fractionDigits must be an integer
  • if set then precision = 10 ** -fractionDigits

Would you allow negative fractionDigits? (multipleOf:10 is probably clearer than fractionDigits:-1)

@ST-DDT
Copy link
Member

ST-DDT commented Dec 30, 2023

Would you allow negative fractionDigits? (multipleOf:10 is probably clearer than fractionDigits:-1)

I'm not sure whether we should actually forbid badly readable input (unless it has other side effects).

@matthewmayer
Copy link
Contributor

So probably allow between -18 and +18 (or whatever range works without issues)

@ST-DDT
Copy link
Member

ST-DDT commented Dec 30, 2023

If we already have a check for the range then we can limit it to useful values. E.g. 0-18

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: feature Request for new feature deprecation A deprecation was made in the PR m: number Something is referring to the number module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Status: Todo
4 participants