Hi Tim, > - The namespace is inconsistently referred to as "BCMath" and "BcMath", > please check the entire RFC to unify the casing.
Thanks, I fixed it. > - The full “stub” should probably include an explicit "implements Stringable" > for clarity. Agree. I describe it explicitly during implementation. > - I don't want to expand the scope of your RFC further than necessary, but > for the rounding mode, I wonder if we should first add the RoundingMode enum > that has been suggested during the discussion of the "Add 4 new rounding > modes to round() function" RFC. It would make the type for the > `Number::$roundMode` property much clearer than the current `int`. I expect > the addition of such an enum to be a pretty simple RFC that would not need > much discussion. I'd be happy to co-author it. Oh, that's a good idea. Makes sense. I think it would be simpler to prepare an RFC separate from this RFC, so I'm going to create one right away. Once you have a certain amount of content, I would be happy if you could check it out and make corrections if necessary. > - The property should probably be named "$roundingMode" instead of > "$roundMode". Thank you, I'm not a native speaker, so pointing out English expressions is very helpful. I fixed it. > - I'm not sure I like the differing default rounding modes for implicit > rounding (i.e. the $roundMode property) and explicit rounding (i.e. the > round() method). That sounds like it would cause unnecessary confusion. I missed the point. All defaults have been changed to HALF_UP. > - In the stub there's a typo: "puclic", should be "public". Oops, thanks! > - For `format()`, I'm not sure whether we should actually add the function. > While we have number_format() for regular numbers with the same signature, it > fails to account for the different language and cultures in the world. > Specifically `number_format()` cannot correctly format numbers for en_IN > (i.e. English in India). In India numbers are not separated every three > digits, but rather the three right-most digits and then every two digits. > Here's in example: 1,23,45,67,890. I believe formatting should be best left > for ext/intl. I'm not very familiar with ext/intl, but is there a way to format a string type number without converting it to a float? > - I'd probably not add the 'with()' method, because it would be redundant > with https://wiki.php.net/rfc/clone_with and it can already be specified by a > combination of withMaxExpansionScale + withRoundMode. I see, I wasn't aware of that argument. Removed `with()`. > - I'm not sure if the priority for the rounding modes is sound. My gut > feeling is that operations on numbers with different rounding modes should be > disallowed or made explicit during the operation (much like the scale for a > division), but I'm not an expert in designing numeric APIs, so I might be > wrong here. As mentioned in the RFC, the problem here is commutative calculations (add, sub, mul). This means that even if specify `$roundingMode` during these calculations, `$roundingMode` will not be used in these calculations. Calculations that use `$roundingMode` are divs, etc. If allow `$roundingMode` to be specified with add, intuitively it feels like the result of add will be rounded. Also, it is not possible to specify `$roundMode` in calculations using operators. However, if the user calculates two numbers with different rounding modes, and it is intentional rather than a mistake, I can't imagine what kind of result the user would want to get. Therefore, it may be better to make this an error. Is it appropriate to throw a value error in that case? > - For the RFC Impact on SAPIs, it should simply be "None", because SAPIs do > not need to do anything special. I fixed it. > - For the Backwards Incompatible Changes, it should list that the > BcMath\Number class will no longer be available to userland. I added that. Thanks! Saki
