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

Change WatchTermination signature to Task<Done> #5647

Merged
merged 1 commit into from Feb 16, 2022

Conversation

ismaelhamed
Copy link
Member

@ismaelhamed ismaelhamed commented Feb 12, 2022

Fixes WatchTermination signature.

Changes

Change WatchTermination signature from Task to Task<Done>. I don't think there's a way to introduce this change without breaking binary compatibility.

Checklist

@ismaelhamed ismaelhamed marked this pull request as ready for review February 12, 2022 10:40
@Aaronontheweb
Copy link
Member

Why is this change needed exactly? Is there a bug that this resolves?

@ismaelhamed
Copy link
Member Author

Why is this change needed exactly? Is there a bug that this resolves?

There's an old PR in JVM where they went from Future to Future[Done] everywhere. I tried to find it to link it here, but couldn't find it. I'll give it another shot.

So now it is impossible to "correctly" write some streams from the JVM in Akka.NET (actually, the other PR I sent goes in the same direction). Also, an empty Task is not good enough to signal completion of streams, for the same reason we return Option<> instead of null.

@Aaronontheweb
Copy link
Member

So now it is impossible to "correctly" write some streams from the JVM in Akka.NET (actually, the other PR I sent goes in the same direction). Also, an empty Task is not good enough to signal completion of streams, for the same reason we return Option<> instead of null.

Got it. Ok. Even though this is a "breaking change" it's minor enough and in an area of the API that is sufficiently low-traffic to a point where I think this is fine.

@Aaronontheweb Aaronontheweb merged commit 05dfaff into akkadotnet:dev Feb 16, 2022
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.

None yet

2 participants