Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Partially revert 4530d3aa93131ec28096968a3c903ed1016dbf1b. Add back v… #1506

Merged

Conversation

marcin-rzeznicki
Copy link
Contributor

…ariance on methods in EitherT and Either syntax.
Current signatures make it impossible to write, for example, for-comprehensions with 'lefts' not being of the same type. The following code does not compile with current signatures, but it compiles after the change (compiler is able to infer the common supertype):

sealed abstract class AppError
case object Error1 extends AppError
case object Error2 extends AppError

val either1: Future[Either[Error1.type , String]] = Future.successful(Right("hi"))
val either2: Future[Either[Error2.type , String]] = Future.successful(Right("bye"))

for {
  s1 <- EitherT(either1)
  s2 <- EitherT[Future, AppError, String](either2)
} yield s1 ++ s2

Error:(17, 7) type mismatch;
found : cats.data.EitherT[scala.concurrent.Future,Error2.type,String]
required: cats.data.EitherT[scala.concurrent.Future, Error1.type,?]
s2 <- EitherT(either2)
^

This change has been discussed on gitter with @travisbrown and @edmundnoble

@codecov-io
Copy link

codecov-io commented Dec 29, 2016

Current coverage is 92.32% (diff: 100%)

Merging #1506 into master will increase coverage by 0.24%

@@             master      #1506   diff @@
==========================================
  Files           246        246          
  Lines          3659       3659          
  Methods        3534       3540     +6   
  Messages          0          0          
  Branches        125        119     -6   
==========================================
+ Hits           3369       3378     +9   
+ Misses          290        281     -9   
  Partials          0          0          

Powered by Codecov. Last update 974b840...5cafcdd

@travisbrown
Copy link
Contributor

👍 from me. We'll need to fix the docs (for example) to match, and I'd prefer to see that happen in this PR, I guess, but it could be a follow-up.

}
}

def partialCompare(that: Either[A, B])(implicit A: PartialOrder[A], B: PartialOrder[B]): Double = eab match {
def partialCompare[AA >: A, BB >: B](that: Either[AA, BB])(implicit AA: PartialOrder[AA], BB: PartialOrder[BB]): Double = eab match {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this method is untested. Is it too much to ask to write a test of some kind during this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not at all. Will do

Copy link
Contributor Author

@marcin-rzeznicki marcin-rzeznicki Dec 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this one should be covered by order laws at https://github.com/typelevel/cats/blob/master/tests/src/test/scala/cats/tests/EitherTests.scala#L44 , right?

EDIT: Ah, it isn't because implementation of partialCompare in EitherInstances is duplicated in EitherOps. Is it better to dedup these two?

def show(implicit A: Show[A], B: Show[B]): String = eab match {
case Left(a) => s"Left(${A.show(a)})"
case Right(b) => s"Right(${B.show(b)})"
def show[AA >: A, BB >: B](implicit AA: Show[AA], BB: Show[BB]): String = eab match {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is untested. Can we add a simple test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course

}

def ap[C](that: Either[A, B => C]): Either[A, C] = (new EitherOps(that)).flatMap(this.map)
def ap[AA >: A, BB >: B, C](that: Either[AA, BB => C]): Either[AA, C] = new EitherOps(that).flatMap(this.map)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is untested. Can we add a simple test?

@marcin-rzeznicki
Copy link
Contributor Author

I'll take care of docs update as well, Thanks for pointing out.

@johnynek
Copy link
Contributor

johnynek commented Dec 30, 2016 via email

@johnynek
Copy link
Contributor

johnynek commented Dec 30, 2016 via email

@marcin-rzeznicki
Copy link
Contributor Author

Missing tests added in 5c86c44

@marcin-rzeznicki
Copy link
Contributor Author

@travisbrown It looks like the whole 'Either in the small...' chapter can be removed from the docs since flatMap you get from enrichment is aligned with what you get on 2.12. WDYT?

@travisbrown
Copy link
Contributor

@marcin-rzeznicki Yep, that makes sense to me.

@travisbrown travisbrown mentioned this pull request Dec 30, 2016
11 tasks
@@ -212,132 +212,6 @@ magic("123") match {
}
```

## Either in the small, Either in the large
Once you start using `Either` for all your error-handling, you may quickly run into an issue where
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this being removed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adelbertc The idea is that all the stuff about the left sides needing to be the same will be a lot less relevant (or not at all relevant, in the form here) once the syntax flatMap for Either behaves the same as the real flatMap method on Either in 2.12.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section talks about using local ADTs for local errors and wrapping as you go wider and wider in your application. Variance is tangentially relevant to Solution 1 but does not help with Solution 2.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, you've convinced me the removal should be more surgical. :)

Copy link
Contributor Author

@marcin-rzeznicki marcin-rzeznicki Dec 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nonetheless, the whole section was presented as a 'solution' - but the problem being solved does not exist anymore. To quote the relevant part: 'If you're on Scala 2.12, this line will compile and work as expected (...)'. Everything that followed seemed to have been written as a guide to deal with issues on previous versions of Scala. It of course does not invalidate ideas contained therein. Obviously, organizing your errors as ADTs is a valid and useful technique. But a rather generic one and not really tied to usage of Either in cats. Of course I'm open to any suggestions, but I must admit that I simply do not know what should be there instead.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should strive to present certain patterns (for lack of a better word) for using these data types - in this case I wrote it to show how one could use Either throughout their code to represent possibly failing computations.

@@ -212,132 +212,6 @@ magic("123") match {
}
```

## Either in the small, Either in the large
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please be more careful about removing this section - variance does not subsume the concerns here.

Deleted section is still relevant - it was meant to present use of `Either` to reprsent failing computations. Variance
or not, it is more aout how to use this data type.
I tried to rewrote parts that are no longer true in presence of variance. Some other parts have been rearranged.
@marcin-rzeznicki
Copy link
Contributor Author

@adelbertc I've come up with a new version of docs. Only some parts that had been no longer true in presence of variance were rewritten. Some other parts were rearranged. Please check if you like it. ( Since Contributors' Guide discourages squashing commits, it may be awkward to compare :-( )

@adelbertc
Copy link
Contributor

Cool @marcin-rzeznicki , LGTM, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants