search

brick / math

Arbitrary-precision arithmetic library for PHP
MIT License
1.83k stars 78 forks source link

Allow both '.' and ',' as decimal separator #75

Closed alessandromorelli closed 10 months ago

alessandromorelli commented 1 year ago
Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Tests pass? yes

The BigNumber::of method is not locale-aware and only accepts a period as a decimal separator.

I propose to allow either a period or a comma as decimal separator, as it would cover all practical use cases, regardless of locale.

BenMorel commented 1 year ago

Hi, the problem is that there are many locales, and that some of them use the comma , as a thousand separator:

https://3v4l.org/IG48Z

So I believe this method should stay locale-agnostic, and that you should create your own parser for this use case!

alessandromorelli commented 1 year ago

Hi, yes, I thought about that, but as the method already forbids thousands separators in the integral part, allowing either as a decimal separator would not generate ambiguity in parsing.

Next option is to allow specifying the separator as an optional parameter to the ::of method, but IMHO it’s not worth the trouble.

Best option would be to have a function to parse a numeric string in its parts, like numberformatter::parse, but that would mean non-userland code.

This change could be a good and useful compromise.

BenMorel commented 1 year ago

as the method already forbids thousands separators in the integral part, allowing either as a decimal separator would not generate ambiguity in parsing.

I don't agree: 1,234 could mean either 1234 or 1.234, depending on the source of the data. I think accepting multiple separators just adds to the confusion. The dot is the standard decimal separator for programming languages, I think that brick/math should support this and nothing more, for the same reason it doesn't accept any thousands separator.

Your use case belongs to a locale-aware number parser, which would be at best a nice addition to brick/math (in a separate class), or even better, a separate package.

BenMorel commented 10 months ago

Closing due to lack of feedback.