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

Commerce price does not return fractional currency prices #350

Closed
ST-DDT opened this issue Jan 29, 2022 · 18 comments · Fixed by #2458
Closed

Commerce price does not return fractional currency prices #350

ST-DDT opened this issue Jan 29, 2022 · 18 comments · Fixed by #2458
Assignees
Labels
c: bug Something isn't working m: commerce Something is referring to the commerce module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Milestone

Comments

@ST-DDT
Copy link
Member

ST-DDT commented Jan 29, 2022

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 ST-DDT added the c: bug Something isn't working label Jan 29, 2022
@ST-DDT
Copy link
Member Author

ST-DDT commented Jan 29, 2022

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

@Shinigami92
Copy link
Member

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 🤔

@Shinigami92
Copy link
Member

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

@damienwebdev
Copy link
Member

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.

@ejcheng
Copy link
Member

ejcheng commented Jan 29, 2022

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 ejcheng moved this to Todo in Faker Roadmap Jan 29, 2022
@ejcheng
Copy link
Member

ejcheng commented Feb 18, 2022

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

@Shinigami92
Copy link
Member

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

@ejcheng ejcheng added this to the v7 - Next Major milestone Feb 18, 2022
@ejcheng ejcheng added s: accepted Accepted feature / Confirmed bug p: 1-normal Nothing urgent labels Apr 7, 2022
@pkuczynski
Copy link
Member

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

@ST-DDT
Copy link
Member Author

ST-DDT commented Jul 3, 2022

IMO we should localize the output some more.

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

@pkuczynski
Copy link
Member

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)

@ejcheng
Copy link
Member

ejcheng commented Jul 3, 2022

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.

@ST-DDT
Copy link
Member Author

ST-DDT commented Jul 3, 2022

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.

@pkuczynski
Copy link
Member

@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

@Shinigami92
Copy link
Member

@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.

@xDivisionByZerox xDivisionByZerox added the m: commerce Something is referring to the commerce module label Jul 30, 2022
@Shinigami92
Copy link
Member

Team decision

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

@Minozzzi
Copy link
Contributor

Minozzzi commented Sep 8, 2022

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

@ST-DDT
Copy link
Member Author

ST-DDT commented Feb 9, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: bug Something isn't working m: commerce Something is referring to the commerce module 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.

7 participants