Skip to content

Conversation

@aturon
Copy link
Contributor

@aturon aturon commented Apr 25, 2018

This PR updates the 0.3 work to match the new RFC.

The approach here keeps the Future trait with an Error type, but uses the new Poll type; an alias PollResult<T, E> = Poll<Result<T, E>> is defined.

The combinators for Future are added back to this branch and updated with the Poll changes, but otherwise are as in 0.2. We'll need follow up with changes to bring them into line, e.g. renaming map to map_ok etc.

My plan is to follow this same approach to get the crates back into working condition (including tests), and then circle back to harmonize the combinators.

@aturon aturon requested a review from cramertj April 25, 2018 05:34

/// An `Async` value represents an asychronous computation.
///
/// An `Asyn` is a value that may not have finished computing yet. This kind of
Copy link
Member

Choose a reason for hiding this comment

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

s/Asyn/Async

/// [executor](../futures_core/executor/trait.Executor.html) as a new, independent
/// task that will automatically be `poll`ed to completion.
///
/// # Combinators
Copy link
Member

@cramertj cramertj Apr 25, 2018

Choose a reason for hiding this comment

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

Here and above you mention combinators on Async-- this should link to AsyncExt.

/// This method is a stopgap for a compiler limitation that prevents us from
/// directly inheriting from the `Async` trait; in the future it won't be
/// needed.
fn poll(self: Pin<Self>, cx: &mut task::Context) -> PollResult<Self::Item, Self::Error>;
Copy link
Member

@cramertj cramertj Apr 25, 2018

Choose a reason for hiding this comment

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

This should include into_async so that AsyncResults can be awaited (well, I suppose it could go on AsyncResultExt, but the method should exist somewhere). Also, i think the name of the poll method here should be poll_result so that it doesn't cause ambiguities.

/// Create a future that is immediately ready with a value.
pub fn ready<T>(t: T) -> ReadyFuture<T> {
ReadyFuture(Some(t))
impl<F> IntoFuture for F where F: Future {
Copy link
Member

@cramertj cramertj Apr 25, 2018

Choose a reason for hiding this comment

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

I think you also want an impl<A> IntoFuture for A where A: AsyncResult + Unpin { ... }.

/// [`err`](::future::err) functions.
#[derive(Debug, Clone)]
#[must_use = "futures do nothing unless polled"]
pub struct FutureResult<T, E> {
Copy link
Member

@cramertj cramertj Apr 25, 2018

Choose a reason for hiding this comment

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

Might as well add an impl of Async + Unpin for FutureResult (same goes for all the other Future-specific combinators).

pub fn never_into<T>(self) -> T {
match self {}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Also want Async for Never.


/// Shorthand for a `Poll<Result<_, _>>` value.
pub type PollResult<T, E> = Poll<Result<T, E>>;

Copy link
Member

Choose a reason for hiding this comment

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

It seems like we should also have a macro here for propagating Pending that works on non-Resulty Polls.

@cramertj
Copy link
Member

The general strategy in this PR takes is to copy all of the combinators over to Future. Is there a reason for organizing it this way, rather than having single combinator types which implement either Async and Future based on their type parameters?

@aturon
Copy link
Contributor Author

aturon commented May 2, 2018

Closing this out, as I'm taking a different approach now.

@aturon aturon closed this May 2, 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.

2 participants