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 WriterT MonadError instance #689

Merged
merged 4 commits into from
Jul 22, 2016

Conversation

ceedubs
Copy link
Contributor

@ceedubs ceedubs commented Nov 21, 2015

This adds a MonadError instance for WriterT. 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]. Calling writerTMonadError[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.

@codecov-io
Copy link

codecov-io commented Nov 21, 2015

Current coverage is 89.53% (diff: 100%)

Merging #689 into master will increase coverage by 0.32%

@@             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          

Sunburst

Powered by Codecov. Last update 4f7b79a...172cf62

@ceedubs ceedubs mentioned this pull request Nov 23, 2015
19 tasks
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))
Copy link

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)

Copy link
Contributor Author

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?

Copy link

@lukiano lukiano May 13, 2016

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.

@yilinwei yilinwei mentioned this pull request May 3, 2016
@non
Copy link
Contributor

non commented May 13, 2016

@ceedubs Is this something you want to keep open? Still working on it?

@ceedubs
Copy link
Contributor Author

ceedubs commented May 13, 2016

@non I'll check if this is found for an F[_] type like Future. If so it may be at least a little useful. I also am interested in whether the Xor example works when the SI-2712 fix plugin is used. I'll get back on this shortly.

@ceedubs
Copy link
Contributor Author

ceedubs commented May 13, 2016

@non @lukiano @kailuowang I just updated this PR. Thanks to @kailuowang's MonadError instance for Option, I can see that this can potentially be useful.

Sorry -- I force pushed a rebase because I had made a mess of things.

@ceedubs
Copy link
Contributor Author

ceedubs commented Jun 2, 2016

@non @lukiano @kailuowang updated again

@kailuowang
Copy link
Contributor

kailuowang commented Jun 2, 2016

LGTM. Did you test Xor with SI-2712 fix? I will be surprised if it doesn't work.

@ceedubs
Copy link
Contributor Author

ceedubs commented Jul 16, 2016

@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 MonadError[WriterT[String Xor ?, Long, ?], String] does indeed resolve just fine with the SI-2712 fix plugin. I think this should be good to merge if the Travis build succeeds.

@ceedubs
Copy link
Contributor Author

ceedubs commented Jul 22, 2016

I've added some (gnarly) tests for the ApplicativeError instance. I look forward to SI-2712 making this a lot cleaner.

@adelbertc
Copy link
Contributor

LGTM 👍

@ceedubs
Copy link
Contributor Author

ceedubs commented Jul 22, 2016

@kailuowang does this still look good to you?

@kailuowang
Copy link
Contributor

👍 LGTM

@kailuowang kailuowang merged commit b9e78f5 into typelevel:master Jul 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants