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

Upgrade to cats-effect, fs2 3.0.0-M3 builds #80

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

Daenyth
Copy link
Contributor

@Daenyth Daenyth commented Oct 8, 2020

This adds some minor api cleanup as well. It shouldn't affect anyone downstream too badly, I think

@Daenyth Daenyth force-pushed the cats-3 branch 4 times, most recently from f19caed to 5d5983e Compare October 8, 2020 14:22
@Daenyth Daenyth marked this pull request as ready for review October 10, 2020 14:07
@Daenyth Daenyth changed the title Upgrade to cats-effect, fs2 3.0.0-M1 builds Upgrade to cats-effect, fs2 3.0.0-M3 builds Nov 21, 2020
@Daenyth
Copy link
Contributor Author

Daenyth commented Nov 23, 2020

I think at this point the main unresolved question is whether CE3 will recommend using Dispatcher as an implicit. Currently it's not really set up for that, but it makes this code clearer to do so. But if the library instructs people to pass it explicitly, I'll want to change the signatures or do the api change like I was thinking earlier

@Daenyth
Copy link
Contributor Author

Daenyth commented Nov 23, 2020

recommendation from CE team is to create locally, it's cheap

https://gitter.im/typelevel/cats-effect-dev?at=5fbbc54afe857d71dc8dad83

Edit: that won't work, as I don't think changing the signature to return F[akka.streams.Source] etc is acceptable. So we will have to take it as an argument

@kubukoz
Copy link
Contributor

kubukoz commented Mar 14, 2021

WDYT about wrapping all ops in a trait and creating a dispatcher as part of that? Then all the methods inside could hopefully return raw streams/sinks (with no constraints, which is a good thing anyway)?

@Daenyth
Copy link
Contributor Author

Daenyth commented Mar 15, 2021

I think it's reasonable, but I wasn't sure. Some thoughts in #84

@Swoorup
Copy link

Swoorup commented Jun 9, 2021

Anything that is blocking this currently? I have been using @Daenyth code is isolation. Seems to do solve my issue since I am using CE3.

@Daenyth
Copy link
Contributor Author

Daenyth commented Jun 9, 2021

I haven't had time to work on this lately, and I'm no longer using it at work.

I'd say this is probably "good enough" to push just to have something out, but I don't have bandwidth to roll it out

Upgrade to fs2 3.2.4 and cats-effect 3.3.1
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