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

Finalizer #3

Merged
merged 4 commits into from Jul 20, 2021
Merged

Finalizer #3

merged 4 commits into from Jul 20, 2021

Conversation

That3Percent
Copy link
Contributor

The main update here is that subscriptions can always observe the final value even if the eventual is closed. This makes a lot more sense than what was happening before (at the expense of some terribly messy internal code).

Also added docs, some minor APIs, and a bump to 0.3.0. I think this should be the last update before putting this to use.

…Handle::forever, Eventual::{from_value, value_immediate}}; Comments
Copy link
Member

@Theodus Theodus left a comment

Choose a reason for hiding this comment

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

LGTM. The documentation improvements are great! Just some minor typos. I find the internal stuff clearer with these changes.

cmp: &Option<Result<T, Closed>>,
cx: &mut Context,
) {
) -> Option<Result<T, Closed>> {
Copy link
Member

Choose a reason for hiding this comment

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

👍

let value = mem::replace(lock.deref_mut(), ChangeVal::None);

match value {
// If there is a new value and it is different than our prevously
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// If there is a new value and it is different than our prevously
// If there is a new value and it is different than our previously

ChangeVal::Finalized(finalized) => {
// Verify that it's not copying the final value over again
// because in racey situations it may have been copied once
// then had the value consumed. It would't be the end of the
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// then had the value consumed. It would't be the end of the
// then had the value consumed. It wouldn't be the end of the

@@ -38,12 +38,8 @@ where
// the future that would produce values. But... that may be very complex. A
Copy link
Member

Choose a reason for hiding this comment

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

Line above: "desireble" -> "desirable"

@That3Percent That3Percent merged commit 9dfb211 into main Jul 20, 2021
@That3Percent That3Percent deleted the finalizer branch July 20, 2021 19:12
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.

None yet

2 participants