Closed non closed 8 years ago
This is currently using a local snapshot. I plan to publish cats 0.6.0-M1 and update this PR to that dependency before asking for a merge. But I wanted to put this out there so folks could take a look at it.
looks good to me.
One comment: I guess we should never add an instance here to any type not defined here if it is only for typeclasses in cats (e.g. Semigroup, Monoid, Group, etc...)
So, for the #148 discussion, we might consider adding the TotalMap
, TotalVector
type to cats kernel? Or just since we added it here, it is safe to add the instances too, of course.
@johnynek
I would be against having any sort of data structure in cats-kernel, even if it is just a trivial wrapper around scala.collection.immutable
data structures. It should be very small and just contain completely uncontroversial typeclasses.
Regarding the implementation, I think TotalX shouldn't have much except an apply method and an O(1) way to get the underlying non-total map/vector.
As far as "vector" wrappers go -- yeah, I think that is a good starting principle.
I think having algebra define simple "vector" wrappers makes sense. I think @rklaehn is right that we should have (unsafe) accessors, but we should also provide a minimal interface to use the type as a vector. The reason I think accessing the underlying structure is (potentially) unsafe is that we want to treat things like Map(1 -> 0.0)
and Map.empty[Double]
as equivalent, but the laws for Eq
mandates that they behave identically.
@non, the way I solve it in https://github.com/rklaehn/abc is that in TotalArrayMap
, I never store mappings to the default value. It's my fetish with canonical representations. Without that property, you wouldn't be able to use structural comparison to compare TotalArrayMap instances, which would suck.
scala> ArrayMap(1 -> 1).withDefault(1)
res1: com.rklaehn.abc.TotalArrayMap[Int,Int] = TotalArrayMap((), 1)
scala> ArraySeq(1,0,1,0).withDefault(0)
res2: com.rklaehn.abc.TotalArraySeq[Int] = ArraySeq(1,0,1).withDefault(0)
A consequence is that sometimes, x.withDefault(y).withoutDefault =!= x
, which might be unexpected.
@rklaehn Right, that is definitely the correct way to do it.
Assuming Travis is happy, I think we should merge this. It depends on a stable Cats milestone (0.6.0-M2) and will allow us to make this transition now anticipating Cats 0.6.0.
some minor comments, but pretty much ready to merge.
I'll go ahead and handle the obvious issues @johnynek pointed out.
👍
👍
:tada:
This commit switches the algebra-core package over to just aliasing the cats-kernel type classes and companion objects. It also uses cats.kernel.std instances instead of the corresponding algebra.std instances.
It also updates the scala.js version to be compatible with cats.kernel, and removes some unnecessary imports.