Conversation
| testFromInt = do | ||
| assert "Zero" $ fromInt 0 == 0.0 | ||
| assert "Negative" $ fromInt (-1) == (-1.0) | ||
| assert "Positive" $ fromInt 1 == 1.0 No newline at end of file |
There was a problem hiding this comment.
Could you add tests for 2 and 5 please? Just in case.
| @@ -0,0 +1,12 @@ | |||
| module Data.Ring.Extra where | |||
There was a problem hiding this comment.
From the outside, it makes little sense to put this function into Data.Ring.Extra rather than just putting it into Data.Ring. What if we move abs and signum from Data.Ord into Data.Ring? I think that would mean that Data.Ord no longer needs to import Data.Ring, which would allow us to use >= here.
I think we should basically inline the definitions of power and Additive too, since importing either of those in Data.Ring will also cause a cycle.
There was a problem hiding this comment.
That'd be a breaking change though, if Data.Ord doesn't export abs/signum anymore.
There was a problem hiding this comment.
Correct, so this would have to wait until the next round of breaking changes. Even though it is breaking, I prefer it over adding a new Data.Ring.Extra module because:
- the API surface makes more sense,
- we'd end up wanting to merge Data.Ring.Extra back into Data.Ring at some point in the future, which would also be breaking, and
- I think it makes more sense to put
absandsignuminto Data.Ring anyway
There was a problem hiding this comment.
Ord also depends on negate in its usage of -1:
instance ordArray :: Ord a => Ord (Array a) where
compare = \xs ys -> compare 0 (ordArrayImpl toDelta xs ys)
where
toDelta x y =
case compare x y of
EQ -> 0
LT -> 1
GT -> -1 -- here it is|
Since it's not clear to me how to resolve this comment #261 (comment) I'm going to say this won't be included in |
Description of the change
Pasta from https://discourse.purescript.org/t/create-a-guide-for-porting-haskell-numbers-to-purescript-numbers/1261/2 as discussed with @hdgarrood on slack.
Checklist: