-
Notifications
You must be signed in to change notification settings - Fork 69
Conversation
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.
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. |
@@ -107,9 +108,9 @@ trait LawTestsBase extends FunSuite with Discipline { | |||
laws[RingLaws, Set[String]].check(_.semiring) | |||
|
|||
laws[OrderLaws, Map[Char, Int]].check(_.eqv) | |||
laws[RingLaws, Map[Char, Int]].check(_.rng) |
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.
did these break because of the change to the Eq
?
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 removed the RNG instance for Map
because of the changes to Eq[Map[K, V]]
.
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 |
I would be against having any sort of data structure in cats-kernel, even if it is just a trivial wrapper around 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 |
@non, the way I solve it in https://github.com/rklaehn/abc is that in 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, |
@rklaehn Right, that is definitely the correct way to do it. |
Instead of the previous SNAPSHOT dependency, this commit moves us to depending on a stable milestone.
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. |
The goal is to make our tests faster. For example, it would be nice if we could eliminate Travis timeouts.
We should re-enable once we figure out how to speed up the JS law tests.
@@ -1,2 +1,38 @@ | |||
package object algebra { | |||
|
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.
would it be too much of a pain to sort the types alphabetically.
some minor comments, but pretty much ready to merge. |
I'll go ahead and handle the obvious issues @johnynek pointed out. |
👍 |
1 similar comment
👍 |
🎉 |
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.