Removed MonadError[Eval, Throwable] #1663
Conversation
|
👍 |
Codecov Report
@@ Coverage Diff @@
## master #1663 +/- ##
==========================================
- Coverage 93.94% 93.93% -0.01%
==========================================
Files 241 241
Lines 4096 4091 -5
Branches 153 151 -2
==========================================
- Hits 3848 3843 -5
Misses 248 248
Continue to review full report at Codecov.
|
|
Shall we address @alexandru's objection here first? |
|
I'm in favor of this change. However, I think that we should give a little more time for discussion on #1661. I propose that we allow a week for discussion and go with what the general consensus seems to be after that amount of time. I also think it would be great if @non could weigh in, since I think that he originally added this code. |
|
|
||
| checkAll("Eval[Int]", MonadErrorTests[Eval, Throwable].monadError[Int, Int, Int]) | ||
| } | ||
| checkAll("Eval[Int]", MonadTests[Eval].monad[Int, Int, Int]) |
There was a problem hiding this comment.
Aren't these MonadTests just a subset of the BimonadTests that are happening above?
Also it seems a little off to me that the SerializableTests for the Bimonad instance are below in a separate scope. This was the case before your PR I think, but I don't know if there's a good reason for it.
There was a problem hiding this comment.
You're correct that MonadTests should just be a subset of BimonadTests. Similar to the SerializableTests, I'm sort of puzzled as to why this was the case before. Mostly I was just carrying it along. If we're all in agreement that it looks fishy though, I'll just zap this line.
There was a problem hiding this comment.
👍 zapping especially if it's confirmed that we don't have a coverage drop after.
|
quick note, the conflict is due to the merge of #1634. |
|
@kailuowang Apparently I had already fixed the conflict, but forgot to push. Pushed now! |
|
Thanks @djspiewak I think we've allowed 10 days of time for people to weigh in. Looks like that the only thing holding is this comment by @ceedubs ? Update: my sign-off is a sign-off modular that comment |
|
There seems to be general (though unfortunately not quite unanimous) consensus on this in #1661, after allowing 10 days or so for discussion. Merging with 4 approvals. |
Fixes a couple outstanding issues. Also obsoletes #1634. See #1661 for an in-depth discussion of why this is necessary.