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

Consider additional Async fns #124

Closed
carllerche opened this issue Sep 3, 2016 · 7 comments
Closed

Consider additional Async fns #124

carllerche opened this issue Sep 3, 2016 · 7 comments

Comments

@carllerche
Copy link
Member

Off the top of my head, it may be useful to provide:

  • unwrap / expect or some variant
  • Conversions between Option<T>.
@ipetkov
Copy link
Contributor

ipetkov commented Sep 3, 2016

If we can convert Async to Option, does Async need to have it's own explicit unwrap/expect functions as well?

Pros of having explicit functions:

  • No need for verbosity of async.into().unwrap()
  • Better panic messages ("tried to unwrap Async result which isn't ready" over "tried to unwrap None value")

Cons:

  • Have to maintain extra functions for parity with Option, list may grow in the future (e.g. map, and_then, etc.)

@alexcrichton
Copy link
Member

I'd personally prefer to avoid these if we can (just cut down on API surface area), but if they're inevitable they're inevitable. My preference would be to go the route of Result and perhaps add a .ready() method returning Option<T>, then you've got all the normal methods.

@ipetkov Result<Option<T>, E> unfortunately got too unwieldy with Stream, where the return type would be Result<Option<Option<T>>, E> and the readability of a match decreased drastically. Having the readable Async<T> in the middle ended up fixing that problem, though.

@ipetkov
Copy link
Contributor

ipetkov commented Sep 6, 2016

@alexcrichton Definitely not advocating against Async, just weighing benefits of duplicating Option APIs on it!

That said, using ready() seems like a good compromise to avoid the manual duplication effort!

@alexcrichton alexcrichton modified the milestone: 0.2 Sep 9, 2016
@alexcrichton
Copy link
Member

I added this to the 0.2 release awhile back, but I don't think it should block it, we can always add functions at any time.

@rphmeier
Copy link

rphmeier commented Dec 28, 2016

I would find an Async::and_then() which is like map but for fallible conversions (deserialization, etc.) useful.

Compare

match inner_future.poll().map_err(Into::into)  {
    Ok(Async::Ready(val)) => transform(val).map(Async::Ready),
    Ok(Async::NotReady) => Ok(Async::NotReady),
    Err(e) => Err(e),
}

to

self.inner_future.poll().map_err(Into::into).and_then(|async| async.and_then(transform))

(where transform is a T -> Result<U, E> and inner_future's Error implements Into<E>)

@stbuehler
Copy link
Contributor

So far and_then functions in the std library returned something with the same base type; Option::and_then returns an Option, Result::and_then returns a Result. Async::and_then would return a Result! This seems unexpected to me.

Maybe go for something similar to what got implemented with Option<Result<T, E>>::transpose:

impl<T, E> Async<Result<T, E>> {
    pub fn transpose(self) -> Result<Async<T>, E> {
        match self {
            Async::Ready(Ok(v)) => Ok(Async::Ready(v)),
            Async::Ready(Err(e)) => Err(e),
            Async::NotReady => Ok(Async::NotReady),
        }
    }
}

Or with an extension trait:

pub trait AsyncResultExt<T, E> {
    fn transpose(self) -> Result<futures::Async<T>, E>;
}

impl<T, E> AsyncResultExt<T, E> for futures::Async<Result<T, E>> {
    fn transpose(self) -> Result<futures::Async<T>, E> {
        match self {
            futures::Async::Ready(Ok(v)) => Ok(futures::Async::Ready(v)),
            futures::Async::Ready(Err(e)) => Err(e),
            futures::Async::NotReady => Ok(futures::Async::NotReady),
        }
    }
}

I'd use it like

self.inner_future.poll()?.map(transform).transpose()

Also see Playground.

@aturon aturon removed this from the 0.2 release milestone Mar 19, 2018
@aturon
Copy link
Member

aturon commented Mar 19, 2018

I'm gonna close this issue -- folks can feel free to open PRs with specific proposals.

@aturon aturon closed this as completed Mar 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants