Closed non closed 9 years ago
I think based on @johnynek's comments I need to go back and improve the Map
instances a bunch.
Another comment I had, but left in the chat room instead, was that we need some GCD tests. Spire has a GCDTest
that does sanity checking, but recently was forced to require Spire's NumberTag
because the Float
/Double
implementations could underflow (which caused NaN
s in LCM). I'd like to try to port the GCD tests over if possible. At least, I'd like to get some tests for the floating point (Float
and Double
for now) implementations, since they're a bit fiddly. I'll give it a try later today.
How do people feel about this PR?
I think it would be a big improvement if we merged it. However, it was maybe too much to bite off in one PR. If folks don't feel comfortable signing-off on this, I'll close it and try to submit individual instances in a more piecemeal fashion.
I'm sorry I let this sit for so long.
/cc @tixxit @avibryant @larsrh @johnynek @ceedubs
(I'm going to go through and try to address as many of @johnynek's comments as I can right now, especially when there is an obvious way to move forward.)
Sorry, I missed this before. Let me look tomorrow and try to merge it.
Just the last two questions, and also not you need to merge in develop.
Thanks for doing all this work. It looks really nice to me.
LGTM.
Can we get a shipit from @tixxit who also looked at this one?
:+1:ship it
Here are a bunch of new instances, some clean ups, etc.
I think all these should be good to go. However:
Vector
)Option
)Semigroup[A] => Semigroup[Option[A]]
butOption[_]
could useorElse
in other cases. Worth supporting?I don't think we have to answer all those questions immediately but we'll eventually want to.