-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 WriterT MonadError instance #689
Conversation
Current coverage is 89.53% (diff: 100%)@@ master #689 diff @@
==========================================
Files 234 234
Lines 3141 3143 +2
Methods 3084 3089 +5
Messages 0 0
Branches 55 51 -4
==========================================
+ Hits 2802 2814 +12
+ Misses 339 329 -10
Partials 0 0
|
def raiseError[A](e: E): WriterT[F, L, A] = WriterT(F0.raiseError[(L, A)](e)) | ||
|
||
def handleErrorWith[A](fa: WriterT[F, L, A])(f: E => WriterT[F, L, A]): WriterT[F, L, A] = | ||
WriterT(F0.handleErrorWith(fa.run)(e => f(e).run)) |
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 wonder if you should append the L from fa.run and the L that results from f(e).run ?
In other words, if the Log that came from fa will be lost (maybe it's ok for it to be forgotten)
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.
@lukiano sorry I'm just seeing this. I don't think we can do what you have recommended. Let's consider Future
as F
(and thus Throwable
as E
). If the Future
fails, then there is no log. We do have a log if the future succeeds, but in that case the function that we pass to handleErrorWith
will never be called, because there is no error to handle.
Does that sound right?
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 you are right. I was thinking of something like XorT -> WriterT -> Future, but in this case F is just Future so there's no monadError instance for it. XorT will provide the MonadError and will already do the right thing.
@ceedubs Is this something you want to keep open? Still working on it? |
@non I'll check if this is found for an |
266665f
to
e2e0e6b
Compare
@non @lukiano @kailuowang I just updated this PR. Thanks to @kailuowang's Sorry -- I force pushed a rebase because I had made a mess of things. |
@non @lukiano @kailuowang updated again |
LGTM. Did you test |
@non @kailuowang sorry about the delay on this one. I've resolved the merge conflict. I just did a check with a locally-published version, and |
I've added some (gnarly) tests for the |
LGTM 👍 |
@kailuowang does this still look good to you? |
👍 LGTM |
This adds a
MonadError
instance forWriterT
. However, it's not very useful in its current form. The following fails to compile, as the instance can't be found:MonadError[WriterT[String Xor ?, Long, ?], String]
. CallingwriterTMonadError[String Xor ?, Long, String]
directly works though.Does anyone know how to convince Scala to find this instance?
Even once it's found, there are some issues with the proper
Arbitrary
instances being found, but I guess we'll take it one step at a time.