-
-
Notifications
You must be signed in to change notification settings - Fork 52
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
feat: improve Js.Int and change some functions to pipe-last #966
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, there are some refs to toPrecisionWithPrecision
that might need updating before merging.
@@ -9,7 +9,7 @@ var suites_0 = [ | |||
return { | |||
TAG: /* Eq */0, | |||
_0: "1.23456e+5", | |||
_1: (123456).toExponential() | |||
_1: (123456).toExponential(undefined) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. This only happens when no labeled args are passed, right? Hopefully it doesn't have any impact on other JS apis 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah. I checked every function to make sure the undefined
argument is not an issue.
In the case of Js.Date
, for example, we need to keep separate functions, as the undefined
argument can make the functions return NaN
.
jscomp/runtime/js_int.ml
Outdated
external toPrecisionWithPrecision : int -> digits:int -> string = "toPrecision" | ||
[@@mel.send] | ||
external toPrecision : ?digits:t -> string = "toPrecision" | ||
[@@mel.send.pipe: t] | ||
(** Formats an [int] using some fairly arbitrary rules | ||
|
||
{b digits} specifies how many digits should appear in total. The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe unrelated to the PR, but the part below is a bit unwarranted imo:
for Node it's 21. Why? Who knows
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -116,32 +78,16 @@ before the decimal point. | |||
@raise RangeError if digits is not in the range accepted by this function (what do you mean "vague"?) | |||
|
|||
{[ | |||
(* prints "1.2e+8" *) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't comment on them, but there are some refs to toPrecisionWithPrecision
that are now obsolete:
melange/jscomp/runtime/js_int.ml
Lines 72 to 76 in 564b968
[toPrecisionWithPrecision] differs from [toFixedWithPrecision] in that the former | |
will count all digits against the precision, while the latter will count only | |
the digits after the decimal point. [toPrecisionWithPrecision] will also use | |
scientific notation if the specified precision is less than the number for digits | |
before the decimal point. |
564b968
to
087e985
Compare
Co-authored-by: Javier Chávarri <javier.chavarri@gmail.com>
No description provided.