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

Add Try instances #1059

Merged
merged 7 commits into from
Jun 1, 2016
Merged

Add Try instances #1059

merged 7 commits into from
Jun 1, 2016

Conversation

johnynek
Copy link
Contributor

suggested a colleague try sequence to convert List[Try[T]] => Try[List[T]] without doing Try(l.map(_.get)) but then didn't see instances for Try.

These are mostly based on Future, but I added Show and Eq.

I made the choice that Eq[Try[T]] considers all Failures as equivalent, which seems better that getting into either taking an Eq[Throwable] or using universal equals, but that can be changed as needed.

@johnynek
Copy link
Contributor Author

PS: I didn't run the tests because when I tried I got some OOM errors, and I figured the CI would need to run anyway.

@guersam
Copy link
Contributor

guersam commented May 24, 2016

Cats doesn't have Try instances because it doesn't obey some typeclass laws such as Monad's. Fortunately, there's an extension for cats named alleycats which provides that kind of instances.

@guersam
Copy link
Contributor

guersam commented May 24, 2016

Or you can use Xor instead of Try with some helpers like Xor.fromTry, Xor.catchNonFatal or etc.: https://github.com/typelevel/cats/blob/v0.6.0/core/src/main/scala/cats/data/Xor.scala#L280-L336

@johnynek
Copy link
Contributor Author

I saw that, but I'm not sure I agree. I don't see any laws violated if only Success and Failure are used like any ADT, and also Future which is included has the same issue.

Can someone clarify this a bit for me?

@johnynek
Copy link
Contributor Author

I guess the difference with Xor is that you have catch happening in each map and flatMap, but this also applies to Future.

To me, Future should be removed, or Try should be added, but the current state does not seem consistent.

@travisbrown
Copy link
Contributor

👍 for including instances for Try in Cats proper.

@stew
Copy link
Contributor

stew commented May 24, 2016

@guersam do we have a demonstration of how the laws are broken? I've usually seen someone trying to show that functor composition is broken when you call fmap with something that isn't even a function, which isn't something that worries me, and I'm not sure there is a difference here in how Xor and Try treat this situation.

@guersam
Copy link
Contributor

guersam commented May 24, 2016

@stew Actually I just naively assumed that @johnynek wasn't aware of alleycats but he already was. I neither am against Try instances in cats.std unless someone brings an anti case. The one found in alleycats uses a partial one like you mentioned, so that I tried to find some previous discussions in gitter but had no luck.

@guersam
Copy link
Contributor

guersam commented May 24, 2016

This conversation seems like the most relevant one https://gitter.im/typelevel/cats?at=54d80e750c939ea03d55764a

@ceedubs
Copy link
Contributor

ceedubs commented May 24, 2016

I agree that if we have Future instances in cats we should also have Try instances. As far as I know, Future has all of the same issues that have been used to discredit Try instances, with the additional annoyance that they require an implicit ExecutionContext.

I'm hesitant about the Eq instance though. Treating all failures as the same, requiring an implicit Eq[Throwable], and using universal equality all seem like bad options. I'd be inclined to either not include the Eq instance or require an implicit Eq[Throwable] with the knowledge that cats won't be providing one so people will have to define their own Throwable equality to opt into this. If we view Try[A] as roughly equivalent to Throwable Xor A, then this would also make the instances mirror each other more closely. If we do treat all failures as the same, then I think it would make sense to also define PartialOrder and Order instances, but it seems that once you start thinking about ordering Throwable things get messy.

@johnynek
Copy link
Contributor Author

Yeah, I'm not 100% about the Throwable thing either. I can move the Eq
instance to the tests. Or as you suggest have it require an Eq[Throwable].

I guess I prefer requiring Eq[Throwable] to removing all together (but then
again, Eq is about the easiest typeclass to implement, so if users have to,
not a disaster).

On Tuesday, May 24, 2016, Cody Allen [email protected] wrote:

I agree that if we have Future instances in cats we should also have Try
instances. As far as I know, Future has all of the same issues that have
been used to discredit Try instances, with the additional annoyance that
they require an implicit ExecutionContext.

I'm hesitant about the Eq instance though. Treating all failures as the
same, requiring an implicit Eq[Throwable], and using universal equality
all seem like bad options. I'd be inclined to either not include the Eq
instance or require an implicit Eq[Throwable] with the knowledge that
cats won't be providing one so people will have to define their own
Throwable equality to opt into this. If we view Try[A] as roughly
equivalent to Throwable Xor A, then this would also make the instances
mirror each other more closely. If we do treat all failures as the
same, then I think it would make sense to also define PartialOrder and
Order instances, but it seems that once you start thinking about ordering
Throwable things get messy.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#1059 (comment)

P. Oscar Boykin, Ph.D. | http://twitter.com/posco | http://pobox.com/~boykin

@ghost
Copy link

ghost commented May 24, 2016

To me, Future should be removed

It will be, see #589 and non/alleycats#26. Unless this changes anything, I propose I do that ASAP (ie can do now)

@ceedubs
Copy link
Contributor

ceedubs commented May 24, 2016

@inthenow: #589 just referred to the Future instances that used Await.result or didn't properly handle failed futures (and was resolved by #1018). At least so far I don't think there's really been any discussion of removing instances such as Monad[Future].

@travisbrown
Copy link
Contributor

I don't care all that much about the instances for Future mentioned in #589 (Comonad, Eq, PartialOrder, Order), but it would be a major annoyance to have to bring in another dependency for the Monad instance for Future.

I think Cats absolutely needs to provide Monad instances for both Future and Try. My vote would be to have the Eq instance for Try require a user-provided Eq[Throwable], and for the Eq and Comonad instances for Future to be non-implicit (and possibly to take a Duration argument or whatever) so that users have to opt in.

@ghost
Copy link

ghost commented May 24, 2016

@ceedubs Correct. But the js versions need to be moved too- And I'll add them both to alleycats

@mpilquist
Copy link
Member

FWIW, I agree with @travisbrown's position.

@johnynek
Copy link
Contributor Author

Can anyone succinctly describe an example of something going wrong when using Try or Future as a "monad". I have heard the concerns about this in the past, but everything I've heard has been about the hidden try/catch that map and flatMap insert. However, when does that cause a user problems? Ordinarily, when we speak about laws, we are concerned with leveraging them to ensure an algorithm is correct or an optimization can be applied.

In what kinds of cases can we imagine the try/catch hidden in Try/Future causing such issues. They may indeed happen, and if that is the case, I tend to be sympathetic that we should have a clearer way to communicate that to users (not sure it should be in a totally separate project as that really harms discoverability and makes it harder to maintain there projects in sync).

Given that all the problems I know about are due to using throw, to which my answer is "don't do that", I am not so concerned with the law violation. The possibility of throw or null are two of the costs of doing business in scala, I would not want to put up more roadblocks to usage and adoption of cats due to issues arising from the use of what we would not consider "best practices".

@ceedubs
Copy link
Contributor

ceedubs commented May 24, 2016

To me it sounds like everyone is pretty much in agreement about keeping around the Monad[Future] instance and that nobody has raised major objections to adding in something similar for Try. I say that we move forward under that assumption. So at this point I think it's basically just a matter of how to handle Eq[Try[A]]. It sounds like there may be a couple votes for requiring a (presumably user-defined) implicit Eq[Throwable] instance.

@ghost
Copy link

ghost commented May 24, 2016

not sure it should be in a totally separate project as that really harms discoverability and makes it harder to maintain there projects in sync

That really raises the question as to whether Alleycats should still exist at all. From gitter chat

But doesn't that suggest that alleycats should be a submodule of cats?
They are quite likely to need to be released in sync.

when we talked about it before people seemed to think that the lawless code should have a firmer separation.
i'm happy to fold the project in if people prefer that design.

@non
Copy link
Contributor

non commented May 24, 2016

The reasoning from @johnynek and @stew makes sense to me.

@non
Copy link
Contributor

non commented May 25, 2016

So, I was writing some tests to see what it would take to beef up our coverage here, and I caught a bug. The Group[Try[A]] defined here is invalid -- there is no inverse for Failure[A] so there can't be a group.

I'm still happy to have this PR, but we should at least remove that instance first.

@milessabin
Copy link
Member

I think this gives us an opportunity to revisit the separation between Cats and Alleycats ... I'd very much like to see the Alleycats classes and instances rolled into Cats proper ... in a submodule if necessary.

@non
Copy link
Contributor

non commented May 25, 2016

@milessabin +1 I was thinking something similar.

@ceedubs
Copy link
Contributor

ceedubs commented May 25, 2016

@milessabin @non @inthenow let's continue the alleycats discussion on #472. I don't want to block progress on this PR by wrapping it up in a separate (albeit related) discussion.

@milessabin
Copy link
Member

@ceedubs I'd completely forgotten about #472 ... sorry for the noise.

@johnynek
Copy link
Contributor Author

I removed the broken Group on this and Future. And I added the needed Eq[Throwable] to get Eq[Try[A]].

@non
Copy link
Contributor

non commented May 26, 2016

LGTM 👍

implicit def showTry[A](implicit A: Show[A]): Show[Try[A]] =
new Show[Try[A]] {
def show(fa: Try[A]): String = fa match {
case Success(a) => s"Success(${A.show(a)})"
case Failure(e) => s"Failure($e)"
}
}
implicit def eqTry[A](implicit A: Eq[A]): Eq[Try[A]] =
/**
* you may with to do equality by making `implicit val eqT: Eq[Throwable] = Eq.allEqual`
Copy link
Contributor

Choose a reason for hiding this comment

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

minor typo: with -> wish

@ceedubs
Copy link
Contributor

ceedubs commented May 30, 2016

Thanks @johnynek this looks great! Sorry, but we've created merge conflicts. Would you be willing to resolve them?

We could also have a Traverse[Try] instance, couldn't we? It's okay if you want to leave that for a separate PR.

@johnynek
Copy link
Contributor Author

johnynek commented Jun 1, 2016

@ceedubs I fixed the merge conflicts but did not yet implement Traverse[Try].

@non
Copy link
Contributor

non commented Jun 1, 2016

Let's merge this now and leave Traverse[Try] for later. 👍

@non non merged commit c43625e into typelevel:master Jun 1, 2016
This was referenced Jun 1, 2016
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.

9 participants