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

Use async fn over a custom Future Type #284

Open
zprobst opened this issue Dec 31, 2020 · 4 comments
Open

Use async fn over a custom Future Type #284

zprobst opened this issue Dec 31, 2020 · 4 comments

Comments

@zprobst
Copy link

zprobst commented Dec 31, 2020

💡 Feature description

I would be willing to perform the change to convert from and get rid of the related cruft as a result. But I wanted to make sure that was a contribution this repo was up to receiving before doing it. This appears to be the most up-to-date and maintained GitHub integration for rust.

💻 Basic example

fn foo() -> Future<T> to an async fn foo() -> Result<T>

@softprops
Copy link
Owner

I'm open to this but since async is sugar for the same thing I'm curious what you believe the net impact would be.

I'll take an example from an existing method.

from

  /// Create a new pull request
    pub fn create(&self, pr: &PullOptions) -> Future<Pull> {
        self.github.post(&self.path(""), json!(pr))
    }

to

  /// Create a new pull request
    pub async fn create(&self, pr: &PullOptions) -> Result<Pull> {
        self.github.post(&self.path(""), json!(pr))
    }

Would you really be gaining much and what would the related cruft amount to?

@zprobst
Copy link
Author

zprobst commented Dec 31, 2020

Sorry! I should have been a bit more clear with my intentions. My motivations are not largely technical. I am mostly thinking about understandability. For those coming from other languages, or at least not familiar with the history Rust's concurrency model, the use of a custom future might be a bit confusing.

In the current documentation, taking your method example above, the documentation says the method returns a Future<Pull>. Clicking that takes you to the docs for the type alias being used. From there, you can parse the type and click on StdFuture. Those docs, intended mostly for people writing futures, has a one sentence description about using .await.

@softprops
Copy link
Owner

I've got you. To fill in missing context async can be used with functions that impl the Std libs Future trait. In the case of hubcaps the reqwest crate provides that impl. Since hubcaps meets async/await contract types you can use that syntax today. Here's an example from the repos list of examples

https://github.com/softprops/hubcaps/blob/master/examples/pulls.rs

@mainrs
Copy link

mainrs commented Mar 8, 2021

I am mostly thinking about understandability.

Ye, I've noticed the same. I usually code without type hints from my editor. Reading the documentation about Labels denotes the return type as Future<Label>. I didn't expect it to be a wrapped Future<Result<Label, Error>>. Having async functions with the "real" return type in the signature makes it easier to work with :)

Maybe a better name would have been FutureResult<T>, but at that point you can also just refactor it into async. I'd gladly do the refactor if it gets merged! :)

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

3 participants