Port Number-related code from deprecated purescript-globals into this repo
#12
Conversation
|
CI passes. Waiting for review. |
src/Data/Number.purs
Outdated
| @@ -5,12 +5,34 @@ module Data.Number | |||
| , isNaN | |||
| , infinity | |||
| , isFinite | |||
| , readInt | |||
There was a problem hiding this comment.
I don't think it makes to export readInt from this module, as readInt doesn't really have much to do with numbers. Additionally, Data.Int already provides a fromString which does what we want.
There was a problem hiding this comment.
While it does provide fromString, readInt can parse a String into a specified base. Should we port this function (albeit under a different name) to purescript-integers? Or should it be dropped altogether?
There was a problem hiding this comment.
That's a good point. I think we should just drop it for now; I think we can add integer parsing in a specified base to purescript-integers if and when someone asks for it.
src/Data/Number.purs
Outdated
| @@ -5,12 +5,34 @@ module Data.Number | |||
| , isNaN | |||
| , infinity | |||
| , isFinite | |||
| , readInt | |||
| , readFloat | |||
There was a problem hiding this comment.
I don't think it makes sense to export this function, as it's just a less safe version of fromString.
src/Data/Number.purs
Outdated
| @@ -5,12 +5,34 @@ module Data.Number | |||
| , isNaN | |||
| , infinity | |||
| , isFinite | |||
| , readInt | |||
| , readFloat | |||
| , toFixed | |||
There was a problem hiding this comment.
I would rather not export toFixed, toExponential, or toPrecision here, since formatting numbers is already covered by Data.Number.Format.
src/Data/Number/Unsafe.purs
Outdated
| -- | | ||
| -- | May throw RangeError if the number of digits is not within the allowed range | ||
| -- | (standard precision range is 0 to 20, but implementations may change it) | ||
| foreign import unsafeToFixed :: Int -> Number -> String |
There was a problem hiding this comment.
I'd prefer not to export these either, since they're more JS-specific in how they handle errors, whereas the core libraries ought to be backend-independent as far as is possible.
There was a problem hiding this comment.
Agreed. Should this be moved to a separate repo outside of core? Or do you think it's fine to drop them entirely?
There was a problem hiding this comment.
I think it's fine to drop them entirely: it's hard for me to imagine a situation where you'd want these functions to be able to throw.
|
I've updated the code to account for @hdgarrood's feedback (thanks!) Let me know whether I need to make any other changes. |
test/Test/Main.purs
Outdated
| import Effect (Effect) | ||
| import Effect.Console (log) | ||
|
|
||
| import Data.Number (isFinite, infinity, | ||
| nan, isNaN, fromString) | ||
| import Data.Number.Format (precision, fixed, exponential, toStringWith, | ||
| toString) | ||
| import Data.Number.Approximate (Fraction(..), Tolerance(..), eqRelative, | ||
| eqAbsolute, (≅), (≇)) |
There was a problem hiding this comment.
It doesn't really matter, but I don't think we format imports like this anywhere else and for consistency's sake it might be nice to format them on one line.
The same is true of these non-ASCII operators from Data.Number.Approximate -- but I suppose we're keeping those due to them being a breaking change to remove?
Either way it's no reason to block this from merging, but I wanted to draw attention here.
There was a problem hiding this comment.
I kept those as is to help show that the port didn't add/remove any code.
Other than that, I agree that the imports should be reduced to one line again. I'll add a commit that does that.
There was a problem hiding this comment.
Yeah, I would usually prefer not to introduce non-ASCII operators in the core libraries, but causing a breaking change by removing them is definitely worse in my book.
There was a problem hiding this comment.
Hm... Yeah, I'm not sure about the non-ASCII syntax.
Backlinking to purescript-deprecated/purescript-globals#22