-
Notifications
You must be signed in to change notification settings - Fork 346
Add Applicative and Functor as superclasses of Monad #330
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
Conversation
Not sure why CI failed. "./sbt test" passes for me locally. |
CI looks green to me? |
Yeah it re-ran for some reason. Some tests marked as flaky? Green now. |
* There are many choices for the canonical second operation (join, sequence, joinWith, ap), | ||
* but these two join and sequence have natural semantics for concurrent | ||
* "MonadError"-like contexts: join fails as soon as one of the elements fails; | ||
* sequence as soon as any fails. Future is the best example of this. |
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.
the explanation of the difference in failure modes between sequence and join is not clear in this comment, imo
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.
Hmm... What I'm trying to say is that join and sequence are good primitives for functors that model concurrent computations. (You could do everything with flatMap but you potentially waste time waiting on operations in order. With join/sequence, if one of them fails, you can fail the entire thing right away.) I can take that part out.
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 think the comment is important, I just found the wording a little muddled.
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.
Better? I'm trying to give a simple and concrete motivation as to why one would care about this typeclass.
/**
- Simple implementation of an Applicative type-class.
- There are many choices for the canonical second operation (join, sequence, joinWith, ap),
- all equivalent. For a functor modelling concurrent computations with failure, like Future,
- combining results with join can save a lot of time vs flatMap. (Given two operations, if the second fails before
- the first completes, one can fail the entire computation right then. If one used flatMap,
- one would have to wait for the first operation to complete before failing it.)
*/
* combining results with join can save a lot of time over combining with flatMap. (Given two | ||
* operations, if the second fails before the first completes, one can fail the entire computation | ||
* right then. With flatMap, one would have to wait for the first operation to complete before | ||
* failing it.) |
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.
This is much clearer. Thank you!
Looks good to me. Unless you have strong feelings to keep genSequence I'd punt on it, let's get the tests green and then I'll give it a once over (or oscar can) and it looks good to merge. |
Ok, removing genSequence. It just makes subclassing harder. (I'm finding the CI to be pretty flaky, failing on different unrelated tests. This branch has been passing locally for me the whole time...) |
If we need to add it back we can And yeah, travis has been acting up recently... very frustrating. I think it might be resource starved. |
Ping (ignore the CI flakiness) |
@johnynek , I'm happy with this patch, though you didn't like the naming of join vs. zip. Thoughts? |
I think this use of the word |
Ping? |
Actually, I think |
Add Applicative and Functor as superclasses of Monad
Thanks! Now let's use it :D |
Well, I tend to bias towards the standard, and I hope Twitter Futures can extends Scala futures one day, and scala futures use zip. Anyway. I guess this is an area of inconsistence, since the scalding Execution monad uses zip as join (and cross on TypedPipe, though I'm not sure what applicative functions will be useful for TypedPipe). |
The choice of Applicative methods is probably the most controversial part of this PR (if it is at all).
I think I maintain source compatibility for everything Monad.scala.