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

Rethink the commerce.price method #2579

Open
ST-DDT opened this issue Dec 14, 2023 · 4 comments
Open

Rethink the commerce.price method #2579

ST-DDT opened this issue Dec 14, 2023 · 4 comments
Labels
breaking change Cannot be merged when next version is not a major release c: bug Something isn't working c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs m: commerce Something is referring to the commerce module p: 1-normal Nothing urgent
Milestone

Comments

@ST-DDT
Copy link
Member

ST-DDT commented Dec 14, 2023

Currently the price method generates prices like this "$5.39". This has several drawbacks, as some currency symbols are written behind the price instead of in front. Some might want an extra space...
There is also the thing about the prices, that require fine tuning of the dec parameter depending on the generated value.
e.g.

// v8.3.1
faker.commerce.price({ min: 0.001, max: 0.009 }) // [Error]: No integer value between 0.001 and 0.009 found.
// #2458
faker.commerce.price({ min: 0.001, max: 0.009 }) // [Error]: No integer value between 0.01 and 0.09 found.
faker.commerce.price({ min: 0.001, max: 0.009, dec: 4 }) // 0.0029

// general
faker.commerce.price({ min: 1_000_000, max: 99_000_000 }) // 3071675.85 (prices like these usually don't have decimal places)

Prices are also very often used as numbers e.g. if they are combined with quantities:

Unit Price Units Total Price
0.29 5 1.45

So commerce.price() should return number.

Summary

  • commerce.price() should return number.
  • the dec parameter should be determined automatically (unless set specifically)
  • the symbol parameter should be removed

#See also

@ST-DDT ST-DDT added c: bug Something isn't working 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 m: commerce Something is referring to the commerce module labels Dec 14, 2023
@ST-DDT ST-DDT added this to the v9.0 milestone Dec 14, 2023
@matthewmayer
Copy link
Contributor

What if the symbol, symbol position and default decimal places came from the locale settings instead?

@ST-DDT
Copy link
Member Author

ST-DDT commented Dec 15, 2023

While I assume that some i18n library/feature might allow that:

  • it would create a dependency on that (the price method would require that feature even if the price is supposed to be without symbol)
  • It requires (additional) parameters to overwrite any defaults that we derive from the locale
  • the user could still call that themselves with the generated price
  • you cannot calculate the total for buying multiple with that price (or have to clean the result somehow)
  • you need to reproduce the formatting again for the total price
  • I assume that whatever currency formatting the GUI uses it is independent of fakers random value generation (it might still be relevant for GUI mocks, but there any currency symbol and placement will do
  • There might be a different or multiple currency symbols regardless of the current locale

I'm not banning the idea, but it should be worth the maintenance overhead.
Maybe we could have two methods one for a number price and one for a "formatted" price.

@matthewmayer
Copy link
Contributor

My feeling is if you remove the currency symbol this is not different enough to faker.number.float

@ST-DDT
Copy link
Member Author

ST-DDT commented Feb 9, 2024

We tried using localization stuff in the past (with Intl) and that caused some issues.
IMO we should let the price method focus on returning prices (numbers).

price({ min: number, max: number, significantDigits?: number, ...}) => number
significantDigits: The number of non zeroed digits. E.g. 5 => 84699000, 132.99, 0.00086489

Then the user can easily pre- or append their symbol and be happy.

Alternatively we could remove the price method, if you think it doesn't do enough.

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: bug Something isn't working c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs m: commerce Something is referring to the commerce module p: 1-normal Nothing urgent
Projects
None yet
Development

No branches or pull requests

2 participants