A mostly clean room reimplementation of Or. by rossabaker · Pull Request #36 · typelevel/cats

rossabaker · 2015-02-02T05:29:46Z

Implements feedback from #11. Specifically:

LOr and ROr move to Or.LeftOr and Or.RightOr.
Constructors are still public, but left and right preserved in
OrFunctions to infer Or. Use what you like.
Only the Scaladoc is ripped from Scalaz.

Also tries to adhere to the emerging consensus on #27.

Preliminary benchmarking showed monomorphism is faster than polymorphism,
and that implementing other operations using fold/bimap is immeasurable.
The second finding is dubious with respect to allocations. If someone
can prove these guilty via benchmark, we can revisit.

I'd like to add tests after #29 happens.

Fixes #11.

Implements feedback from #11. Specifically: 1. LOr and ROr move to `Or.LeftOr` and `Or.RightOr`. 2. Constructors are still public, but `left` and `right` preserved in `OrFunctions` to infer `Or`. Use what you like. 3. Only the Scaladoc is ripped from Scalaz. Also tries to adhere to the emerging consensus on #27. Preliminar benchmarking showed monomorphism is faster than polymorphism, and that implementing other operations using fold/bimap is immeasurable. The second finding is dubious with respect to allocations. If someone can prove these guilty via benchmark, we can revisit. I'd like to add tests after #29 happens.

non · 2015-02-02T06:07:09Z

This looks great!

If you wouldn't mind adding toOption and toEither I think most users would appreciate having those available. I think the generic to method is good too, but for those common interop cases I think it's nice to have explicit methods.

tpolecat · 2015-02-02T06:20:29Z

data/src/main/scala/cats/data/Or.scala

Suggest always doing this for ADTs to get a better LUB when unifying constructors.

sealed abstract class Or[+A, +B] extends Product with Serializable {

@tpolecat do you have an example of the benefit where it is helpful to add extends Product with Serializable ?

scalaz/scalaz#450

For abstract classes at the root of ADTs, I've added these silly supers to the root. Here's why. It's even worse than that: the inferred types look like Product with Serializable with Box[B], and they erase to Object, not Box[B], in the stored Java types.
The benefit is improved inference when using case classes explicitly; the drawback is that Product methods appear in the type itself. So keep this, remove it, whichever you like.

@julien-truffaut thus:

sealed trait A case class Aa() extends A case class Ab() extends A scala> List(Aa(), Ab()) res1: List[Product with Serializable with A] = List(Aa(), Ab()) sealed trait A extends Product with Serializable case class Aa() extends A case class Ab() extends A scala> List(Aa(), Ab()) res2: List[A] = List(Aa(), Ab()) // LUB is better

@tpolecat cool I didn't know this trick!

I was sitting next to @tpolecat 3 days ago when we both learned this trick during Jon Pretty's nescala talk ;)

non · 2015-02-02T06:24:22Z

@tpolecat Those are great suggestions. I keep mentioning this contributor's guide that doesn't exist, but those would be great things to add to it somewhere.

tpolecat · 2015-02-02T06:56:40Z

data/src/main/scala/cats/data/Or.scala

This is interesting. Why is this is done with syntax rather than a method with a type constraint? i.e., something like

def merge[X >: A](implicit ev: B <:< X]): X = ...

Apologies if I missed an earlier discussion about this.

scalaz/scalaz#580 discussion in scalaz

Conflicts: data/src/main/scala/cats/data/Or.scala

This lets us override map just once.

non · 2015-02-02T17:32:56Z

This looks good. 👍

One thing that we did in algebra that I'd like to try to do here is to ensure our type class instances are Serializable. This is a godsend to anyone who wants to write spark or Hadoop jobs and might end up capturing type class instances.

I think we should merge this PR before doing that, but I just wanted to mention it here.

stew · 2015-02-02T17:42:32Z

yeah I agree on the serializable stuff, that's coming up for us with spark.

julien-truffaut · 2015-02-02T17:46:11Z

concretely what does it mean? Do we simply need to extends all TC with Serializable?

non · 2015-02-02T17:48:10Z

Yes, and the instances need to do this as well.

I actually have a rule for law-checking which confirms an instances is serializable:

https://github.com/non/algebra/blob/master/laws/src/main/scala/algebra/laws/Rules.scala#L12

tpolecat · 2015-02-02T17:50:48Z

Yep if it matters then it needs to be part of the base test for all typeclasses; Serializable is just a marker of intent, it doesn't prove anything.

mpilquist · 2015-02-02T17:50:59Z

Should simulacrum do this automatically? I was already considering making simulacrum change all type class traits to universal traits.

non · 2015-02-02T17:51:43Z

@mpilquist If you are open to it then I think it would be great. I don't see a good reason anyone should specifically want their type classes to be non-serializable.

(But of course we still have to remember to extend serializable in the instances too.)

julien-truffaut · 2015-02-02T17:52:32Z

pardon the stupid question but why do you need the instance to extends Serializable if the TC already does?

non · 2015-02-02T17:54:06Z

@julien-truffaut I'm not sure 100% sure why but I think the Serializable trait is somewhat special and functions a little bit like an annotation. Maybe someone else here has a better understanding, but I know that until algebra instances extended it themselves they were failing the test I linked.

(Also if someone can demonstrate it working the "normal" way then that would be ideal. I'm just reporting my own experience -- I may have messed something up.)

tpolecat · 2015-02-02T17:55:10Z

Huh. In Java you can inherit it, but maybe it gets squashed out in the trait encoding?

johnynek · 2015-02-02T19:21:30Z

data/src/main/scala/cats/data/Or.scala

do we need the Or suffix on these names given they are namespaced by the Or object already?

It is convenient when the name of the class is unique when you use auto import

Agree. It's really easy to run into trouble when you're shadowing names that are already in scope by default. Common point of bafflement in #scala for people in the Coursera course, which has exercises to implement List and Set.

julien-truffaut · 2015-02-03T08:27:36Z

data/src/main/scala/cats/data/Or.scala

it is really a details but I think it reads slightly better if you pattern match first with LeftOr since the first function is on A

This was an almost certainly premature optimization, figuring there'd be more rights than lefts. I'll flip them for readability.

julien-truffaut · 2015-02-03T08:32:01Z

can you add a show method? and implement Show instance in terms of it

non · 2015-02-04T16:56:45Z

I think we should prefer new Show[...] { ... } instead of Show.show[...](...) in cats.

The reason is that while both allocate Show instances the latter also creates an unnecessary Function1 instance. Maybe it's a bit premature but if we're going to have a standard I'd prefer to use this one.

julien-truffaut · 2015-02-04T17:16:54Z

@non good for me, we should also add this to the contributor guide.

non · 2015-02-04T23:45:27Z

I think I'm pretty happy with this. 👍

Anyone know a reason why we shouldn't merge this and continue future work in another PR?

mpilquist · 2015-02-04T23:50:05Z

👍

A mostly clean room reimplementation of Or.

tpolecat reviewed Feb 2, 2015
View reviewed changes

rossabaker added 4 commits February 2, 2015 10:34

Update Or according to feedback.

23a7d37

Merge remote-tracking branch 'origin/master' into topic/or-rewrite

061e309

Conflicts: data/src/main/scala/cats/data/Or.scala

Split out orPartialOrder, orEq so as to require the minimal instance.

9ae4437

Consolidate Traverse and Monad instances.

ee6036e

This lets us override map just once.

johnynek reviewed Feb 2, 2015
View reviewed changes

stew added the in progress label Feb 2, 2015

julien-truffaut reviewed Feb 3, 2015
View reviewed changes

rossabaker added 2 commits February 3, 2015 11:35

Flip LeftOr/RightOr cases; add Or.show.

d17c9af

Save an allocation in Or.filter.

1b02a4e

With @mpilquist's help, implementation of fromTryCatch for Or

be29556

Fix compilation error from escaping private class.

bb26aee

non added a commit that referenced this pull request Feb 4, 2015

Merge pull request #36 from rossabaker/topic/or-rewrite

16fafd9

A mostly clean room reimplementation of Or.

non merged commit 16fafd9 into typelevel:master Feb 4, 2015

non removed the in progress label Feb 4, 2015

rossabaker deleted the topic/or-rewrite branch February 5, 2015 00:19

Uh oh!

Conversation

rossabaker commented Feb 2, 2015

Uh oh!

non commented Feb 2, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

non commented Feb 2, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

non commented Feb 2, 2015

Uh oh!

stew commented Feb 2, 2015

Uh oh!

julien-truffaut commented Feb 2, 2015

Uh oh!

non commented Feb 2, 2015

Uh oh!

tpolecat commented Feb 2, 2015

Uh oh!

mpilquist commented Feb 2, 2015

Uh oh!

non commented Feb 2, 2015

Uh oh!

julien-truffaut commented Feb 2, 2015

Uh oh!

non commented Feb 2, 2015

Uh oh!

tpolecat commented Feb 2, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julien-truffaut commented Feb 3, 2015

Uh oh!

non commented Feb 4, 2015

Uh oh!

julien-truffaut commented Feb 4, 2015

Uh oh!

non commented Feb 4, 2015

Uh oh!

mpilquist commented Feb 4, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants