faker icon indicating copy to clipboard operation
faker copied to clipboard

Commerce price does not return fractional currency prices

Open ST-DDT opened this issue 3 years ago • 18 comments

Describe the bug

faker.commerce.price() always returns a value with full price. => 245.00 , 842.00

It would be nice if it could also generate prices with fractional currency prices => 245.71 , 842.45

Also if invalid prices ranges are given, then the returned prices does not respect the decimal places paramter.

Reproduction

faker.commerce.price() => 245.00 faker.commerce.price(-1) => 0

Additional Info

No response

ST-DDT avatar Jan 29 '22 15:01 ST-DDT

@Shinigami92 Would you consider this a bug or feature request?

ST-DDT avatar Jan 29 '22 15:01 ST-DDT

I think the precision could be a bug, but maybe it was considered as wanted cause if you have e.g. an app that shows some prices, the prices are just xx.00

What I think would be a better idea is to directly create a whole new module for currency :thinking: Also we should take stuff into account like the symbol could be placed on the right or left side: $12.50 or 12,50 €


I think we could do 2-3 PRs targeting different versions, on the other side we should ask some folks if the .00 is expected currently :thinking:

Shinigami92 avatar Jan 29 '22 15:01 Shinigami92

We could also try to play around with https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/NumberFormat#using_options :eyes:

Shinigami92 avatar Jan 29 '22 15:01 Shinigami92

A comment here on pricing, it's not safe to assume that all prices are formatted to two decimals.

E.g. in the US, prices for gasoline are $2.999/gallon.

Even within a singular currency, there are inconsistencies.

damienwebdev avatar Jan 29 '22 15:01 damienwebdev

I think the precision could be a bug, but maybe it was considered as wanted cause if you have e.g. an app that shows some prices, the prices are just xx.00

What I think would be a better idea is to directly create a whole new module for currency 🤔 Also we should take stuff into account like the symbol could be placed on the right or left side: $12.50 or 12,50 €

I think we could do 2-3 PRs targeting different versions, on the other side we should ask some folks if the .00 is expected currently 🤔

Yes, I think a new module would be the best approach here, too.

ejcheng avatar Jan 29 '22 17:01 ejcheng

@Shinigami92 should this be put under v6.2 or v7? thanks

ejcheng avatar Feb 18 '22 15:02 ejcheng

To get some more prio to other features, I think we can put this into v7 for now

Shinigami92 avatar Feb 18 '22 17:02 Shinigami92

I would suggest dropping this function as we have datatype.float, which takes the precision parameter. WDYT?

pkuczynski avatar Jul 03 '22 19:07 pkuczynski

IMO we should localize the output some more.

E.g. de -> 1,99€ en_us -> $1.99

ST-DDT avatar Jul 03 '22 20:07 ST-DDT

I think currency should not come with a price. I might want to stay on pl-PL locale and still generate prices in €... Should I switch faker to de just to get the symbol? Don't think so. Currency symbols should not be based on locale. $ is the same in any locale. We can add faker.money.currency or faker.commerce.currency. We should also support short and long. However I think for adding currency users should use intl module (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/NumberFormat/NumberFormat)

pkuczynski avatar Jul 03 '22 21:07 pkuczynski

I would suggest dropping this function as we have datatype.float, which takes the precision parameter. WDYT?

I'm not sure, I could honestly go either way on this. We could have this function return values with currency symbols, which would be nice, but datatype.float still pretty much does the same thing, and users could use intl to add the symbols.

ejcheng avatar Jul 03 '22 22:07 ejcheng

I haven't worked with that api yet, but it looks straight forward. Is it well known in the JS community? If yes, then we can drop price. If not, then we should consider how we can guide our users to that function/API (Maybe retaining the method as alias to float, but with extended Jsdocs.

ST-DDT avatar Jul 03 '22 23:07 ST-DDT

https://caniuse.com/?search=intl%20currency

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/NumberFormat#browser_compatibility

Shinigami92 avatar Jul 04 '22 05:07 Shinigami92

@Shinigami92 what would be your suggestion then? price returns the string and fills with 0 if not enough precision is generated. Additionally, it offers to add a currency sign.

https://fakerjs.dev/api/commerce.html#price

pkuczynski avatar Jul 04 '22 08:07 pkuczynski

@Shinigami92 what would be your suggestion then? price returns the string and fills with 0 if not enough precision is generated. Additionally, it offers to add a currency sign.

fakerjs.dev/api/commerce.html#price

I must say, I don't know I currently don't have strong opinions of how to design this or what would be good UX/DX Just wanted to link on which platforms Intl can be used and it looks like we are safe with Intl because it runs in Node and all ever green browsers. Maybe we could wrap it and use it in Faker itself? Maybe we move the headaches to the user itself 🤷😅 I can assume that the commerce.price functionality is and was a good usable function to directly get what you asked for and show e.g. some fake prices in your mocked web-store. I don't and never did in the past worked with a store, so I don't have any experience on that section.

Shinigami92 avatar Jul 04 '22 08:07 Shinigami92

Team decision

We want to change the implementation of the price method to use Intl directly/internally

Shinigami92 avatar Sep 08 '22 17:09 Shinigami92

I would like to try to solve this task. Could assign to me?

Minozzzi avatar Sep 08 '22 17:09 Minozzzi

FFR: We decided (some time ago) that we tackle the issue as is in #2458 and rethink the implementation later in #2579

  • https://github.com/faker-js/faker/pull/2458#issuecomment-1856253990
  • #2579

ST-DDT avatar Feb 09 '24 18:02 ST-DDT