Decimal.round updates #16
Conversation
| }, | ||
| }; | ||
|
|
||
| const checkAndInitMathHandlers = (createUnaryHandler, createNaryHandler) => { |
There was a problem hiding this comment.
Rather than leave this code unwrapped in patches.js, I think this check made it worth moving into its own file.
| throw new Error(`Programmer error: patched Math method missing`); | ||
| } | ||
|
|
||
| PATCHED_MATH_METHODS.forEach((method) => { |
There was a problem hiding this comment.
I never love doing this — the side-effecty forEach, the abstraction from the list of methods / table of contents approach, but I do think the handler check is probably worth it in this case.
| }; | ||
|
|
||
| function round(decimal, options = Object.create(null)) { | ||
| const round = (decimal, options = {}) => { |
There was a problem hiding this comment.
Just updated this to match the way we are instantiation functions here. It's better to keep it consistent.
|
|
||
| if (maximumSignificantDigits !== undefined) { |
There was a problem hiding this comment.
The proposal does not currently support maximumSignificantDigits and I would rather keep it simple unless we hear a lot of requests for it.
There was a problem hiding this comment.
I agree that we shouldn't add things that are not in the proposal, but where are you seeing that it's not part of the proposal? There's no section header to link directly to, but it's in the Common elements section if you scroll down a bit.
There was a problem hiding this comment.
Ughhhh it's not listed in the specific references. I'm honestly tempted to leave it out because I don't really see the obvious need for two slightly different ways to round. And if there is clamor for it in its absence we've leaved something. At least through the magic of version control, the code is still referenceable so we haven't wasted your work.
There was a problem hiding this comment.
Sounds good. I've opened tc39/proposal-decimal#74 to audit for more of these discrepancies.
| decimal, | ||
| maximumSignificantDigits, | ||
| internalMode | ||
| if (!(coercedFractionDigits >= 0)) { |
There was a problem hiding this comment.
I've changed from the check from undefined and then coercion with the unary plus to Number coercion and then the value because this way it catches null and empty strings and all sorts of things before the check.
| }, | ||
| }); | ||
|
|
||
| const decimalOnlyBaseFn = (fnName) => () => { |
There was a problem hiding this comment.
Using the error-thrower as the target function is definitely inspired. I've abstacted it out here both so that we can use the name to explain what it's doing a bit and because I expect it's useful enough we'll see it again.
| throw new Error( | ||
| `${what} not yet supported for ${implName}. Let us know if you need this! (https://github.com/tc39/proposal-decimal)` | ||
| `${what} is not yet supported for ${implName}. Let us know if you need this! (https://github.com/tc39/proposal-decimal)` |
There was a problem hiding this comment.
I also really love this error. Thank you!
|
@ptomato FYI ⬆️ |
I merged #14 earlier and then added my changes here instead of going back and forth. I am going to merge these as well, but feel free to comment on the PR still. Tomorrow my plan is to get actions up and building so we don't have to manually build and then see what I can add for the rest of the week.