Skip to content

Add IO#redeem and IO#redeemWith#191

Merged
alexandru merged 4 commits into
typelevel:masterfrom
alexandru:redeem
May 4, 2018
Merged

Add IO#redeem and IO#redeemWith#191
alexandru merged 4 commits into
typelevel:masterfrom
alexandru:redeem

Conversation

@alexandru

@alexandru alexandru commented Apr 25, 2018

Copy link
Copy Markdown
Member

This is related to typelevel/cats#2237

This uses IO's internals to expose an optimized redeemWith operation, that's equivalent with doing attempt.flatMap, but avoiding a second flatMap boundary, or extra boxing in Either.

The performance improvements on doing this are pretty good, see: typelevel/cats#2161 — and the capability of doing this has been in IO ever since I optimized its internal encoding, we just need to expose it.

@codecov-io

codecov-io commented Apr 25, 2018

Copy link
Copy Markdown

Codecov Report

Merging #191 into master will increase coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #191      +/-   ##
==========================================
+ Coverage   89.63%   89.69%   +0.05%     
==========================================
  Files          50       50              
  Lines        1081     1087       +6     
  Branches       82       77       -5     
==========================================
+ Hits          969      975       +6     
  Misses        112      112

@LukaJCB LukaJCB left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me! Sounds pretty useful in general :)

@alexandru

Copy link
Copy Markdown
Member Author

Actually I want to think about this some more, so I'm closing it to prevent accidental merging. Will reopen if the case.

@alexandru alexandru closed this Apr 25, 2018
@alexandru

Copy link
Copy Markdown
Member Author

OK, thought about it 🙂

As mentioned in typelevel/cats#2237:

I've carefully considered possibilities, doing benchmarks with alternatives, like a variant that takes a function with an Either[Throwable, A] parameter, plus I modified some Monix code to see how it feels like and I think this approach is the best.

The signature is nice too, it's basically a fold (catamorphism?) that happens in the F[_] context.

Unfortunately for abstracting over it we'll have to wait for Cats 2.0 I think.
Or maybe introduce a MonadRedeemBinCompat in Cats 1.x.

@alexandru alexandru reopened this Apr 27, 2018

@LukaJCB LukaJCB left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

@alexandru

Copy link
Copy Markdown
Member Author

Got verbal confirmation that @LukaJCB and @kailuowang are interested in approving the related PR in Cats.

I think it's safe to move forward with it and merge this.

@LukaJCB

LukaJCB commented May 4, 2018

Copy link
Copy Markdown
Member

Sounds good to me! Though Travis is failing with a syntax error :)

@alexandru

Copy link
Copy Markdown
Member Author

Broken merge with master, I fixed it.

@LukaJCB LukaJCB left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

@alexandru
alexandru merged commit 289495b into typelevel:master May 4, 2018
@alexandru alexandru added this to the 1.0.0-RC2 milestone May 23, 2018
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.

5 participants