API: substitute `Result<Future..>` with `Future..` for more idiomatic future chaining #49

steveej · 2018-10-17T14:46:11Z

This change is motivated by an out-of-band discussion with @lucab and a comment on a related PR openshift/cincinnati#4 (comment) by @crawford, suggesting to use future chaining.

Before this PR all methods return a Result<Future..> chaining them requires to unwrap() or slightly milder unwrap_or_else() for every additional chain element, which adds significant boilerplate for the consumer. This PR aims to aid with this by removing the outer Result<Future..> and return Future.. for all async public methods.

I'd like to discuss this API breaking change with the intention to move forward with this or a similar approach for the whole crate. I've done a rewrite of the login example demonstrates the usage of the changed API.

Rewrite all of the examples to use the new API
Rewrite examples to use chaining
- ~~examples/checkregistry.rs~~
- examples/image.rs
- examples/login.rs
- examples/tags.rs
- examples/trace.rs
Rewrite all of the? API

steveej · 2018-10-17T20:09:53Z

src/v2/auth.rs

        let subclient = self.hclient.clone();
        let creds = self.credentials.clone();
        let scope = scopes
            .iter()
            .fold("".to_string(), |acc, &s| acc + "&scope=" + s);
        let auth = self
-            .get_token_provider()?
+            .get_token_provider()
+            .unwrap()


steveej · 2018-10-17T20:10:04Z

src/v2/mod.rs

-        let url = try!(hyper::Uri::from_str(
-            (self.base_url.clone() + "/v2/").as_str()
-        ));
+        let url = hyper::Uri::from_str((self.base_url.clone() + "/v2/").as_str()).unwrap();


lucab · 2018-10-17T20:17:08Z

I would say that yes, we should try to drop the outer Result<> and convert the error case into an inner future result. The current API is my first attempt at a futures-based library, as you can see...

steveej · 2018-10-23T19:45:12Z

I've changed all methods referenced by the examples. A first review pass would make sense now ;-)

lucab · 2018-10-24T09:27:32Z

examples/image.rs

+        .and_then(|dclient| {
+            dclient.is_v2_supported().and_then(|v2_supported| {
+                if !v2_supported {
+                    return Err("API v2 not supported".into());


You can drop the explicit return here (or flatten the else branch).

lucab · 2018-10-24T09:28:24Z

examples/image.rs

+        }).and_then(|dclient| {
+            dclient.is_auth(None).and_then(|is_auth| {
+                if is_auth {
+                    return Err("no login performed, but already authenticated".into());


Same as above.

lucab · 2018-10-24T09:32:19Z

examples/image.rs

+                        }
+                        dkregistry::mediatypes::MediaTypes::ManifestV2S2 => {
+                            let m: dkregistry::v2::manifest::ManifestSchema2 =
+                            serde_json::from_slice(manifest_body.as_slice()).unwrap();


There are two leftover unwraps here and in the match-arm above.

examples/image.rs

lucab · 2018-10-24T09:35:37Z

examples/login.rs

 extern crate tokio_core;

 use std::{boxed, error};
 use tokio_core::reactor::Core;

+use futures::future::Future;


Just use the prelude here too.

lucab · 2018-10-24T09:37:22Z

examples/tags.rs

+            })
+        }).and_then(|dclient| {
+            dclient
+                .login(&[&format!("repository:{}:pull", image)])


I would extract the scope-formatting outside (before) the start of the chain, so that it's harder for people to miss (here and everywhere else too).

lucab · 2018-10-24T09:38:40Z

examples/tags.rs

+                            } else {
+                                println!("logged in!");
+                                Ok(dclient
+                                    .clone()


Is this clone really needed?

Yes, because:

error[E0596]: cannot borrow immutable captured outer variable in an `FnOnce` closure `dclient` as mutable --> examples/tags.rs:77:36 | 77 | Ok(dclient | ^^^^^^^ error: aborting due to previous error For more information about this error, try `rustc --explain E0596`. error: Could not compile `dkregistry`.

lucab · 2018-10-24T09:39:42Z

src/v2/blobs.rs

        let cl = self.clone();
        let url = {
            let ep = format!("{}/v2/{}/blobs/{}", self.base_url.clone(), name, digest);
-            hyper::Uri::from_str(ep.as_str())?
+            hyper::Uri::from_str(ep.as_str()).unwrap()


Leftover unwrap (there are a few more below without a fixme note).

Before this commit all methods return a `Result<Future..>` chaining them requires to `unwrap()` or slightly milder `unwrap_or_else()` for *every* additional chain element, which adds significant boilerplate for the consumer. This commit aims to aid with this by removing the outer `Result<Future..>` and return `Future..` for all async public methods.

lucab · 2018-10-24T22:11:05Z

src/v2/manifest/mod.rs

            "{}/v2/{}/manifests/{}",
            self.base_url.clone(),
            name,
            reference
-        )));
+        )).unwrap();


Leftover unwrap.

lucab · 2018-10-24T22:11:12Z

src/v2/manifest/mod.rs

-                .append(header::ACCEPT, header::HeaderValue::from_str(&mtype)?);
+            r.headers_mut().append(
+                header::ACCEPT,
+                header::HeaderValue::from_str(&mtype).unwrap(),


Leftover unwrap.

lucab · 2018-10-24T22:11:19Z

src/v2/manifest/mod.rs

        let url = {
            let ep = format!(
                "{}/v2/{}/manifests/{}",
                self.base_url.clone(),
                name,
                reference
            );
-            try!(hyper::Uri::from_str(ep.as_str()))
+            hyper::Uri::from_str(ep.as_str()).unwrap()


Leftover unwrap.

lucab · 2018-10-24T22:11:26Z

src/v2/manifest/mod.rs

        };
        let accept_types = match mediatypes {
            None => vec![mediatypes::MediaTypes::ManifestV2S2.to_mime()],
-            Some(ref v) => try!(to_mimes(v)),
+            Some(ref v) => to_mimes(v).unwrap(),


Leftover unwrap.

lucab · 2018-10-24T22:11:38Z

src/v2/tags.rs

        let url = {
            let mut s = format!("{}/v2/{}/tags/list", self.base_url, name);
            if let Some(n) = paginate {
                s = s + &format!("?n={}", n);
            };
-            try!(hyper::Uri::from_str(s.as_str()))
+            hyper::Uri::from_str(s.as_str()).unwrap()


Leftover unwrap.

lucab · 2018-10-24T22:13:14Z

@steveej this looks pretty good to me. There are only five leftover unwraps and then I think it's done.

steveej · 2018-10-25T09:00:01Z

Yay, more unwraps to clear up 🎂 Thanks for catching these

lucab

LGTM

`git grep -e 'unwrap()' --and --not -e '^//' -n src/` doesn't indicate any occurences left.

steveej force-pushed the return-future-without-result-and-work-examples branch from 2f36b4c to 2a18364 Compare October 17, 2018 14:48

steveej changed the title ~~[WIP] return-future-uncapsuled-and-rewrite-examples~~ [WIP/RFC] API: substitute Result<Future*> with Future* for more idiomatic future chaining Oct 17, 2018

steveej commented Oct 17, 2018

View reviewed changes

steveej changed the title ~~[WIP/RFC] API: substitute Result<Future*> with Future* for more idiomatic future chaining~~ [WIP/RFC] API: substitute Result<Future..> with Future.. for more idiomatic future chaining Oct 17, 2018

steveej force-pushed the return-future-without-result-and-work-examples branch 4 times, most recently from 4f5a625 to d1bdb2a Compare October 23, 2018 18:14

steveej requested a review from lucab October 23, 2018 19:45

steveej force-pushed the return-future-without-result-and-work-examples branch 2 times, most recently from 64255ef to 8b7b13f Compare October 24, 2018 09:20

lucab reviewed Oct 24, 2018

View reviewed changes

steveej force-pushed the return-future-without-result-and-work-examples branch 6 times, most recently from d4199d0 to 10aafe8 Compare October 24, 2018 21:08

steveej force-pushed the return-future-without-result-and-work-examples branch from 10aafe8 to 363baf3 Compare October 24, 2018 21:31

steveej changed the title ~~[WIP/RFC] API: substitute Result<Future..> with Future.. for more idiomatic future chaining~~ API: substitute Result<Future..> with Future.. for more idiomatic future chaining Oct 24, 2018

steveej force-pushed the return-future-without-result-and-work-examples branch from 363baf3 to 8ac4299 Compare October 24, 2018 21:35

steveej requested a review from lucab October 24, 2018 21:36

steveej mentioned this pull request Oct 24, 2018

v2/tags: follow results on multiple pages #48

Merged

lucab reviewed Oct 24, 2018

View reviewed changes

steveej force-pushed the return-future-without-result-and-work-examples branch from 8498ddc to 8452a89 Compare October 25, 2018 10:05

steveej force-pushed the return-future-without-result-and-work-examples branch 3 times, most recently from d2bf4ac to a1fc31b Compare October 25, 2018 13:17

steveej requested a review from lucab October 25, 2018 15:05

lucab approved these changes Oct 25, 2018

View reviewed changes

steveej added 2 commits October 25, 2018 19:01

examples/*: streamline, refactor, and rewrite to use future chaining

c42c320

API: resolve all remaining unwrap()s

8f0dabe

`git grep -e 'unwrap()' --and --not -e '^//' -n src/` doesn't indicate any occurences left.

steveej force-pushed the return-future-without-result-and-work-examples branch from a1fc31b to 8f0dabe Compare October 25, 2018 17:01

examples/tags: add loop demo

edfc08b

steveej merged commit 23adf75 into camallo:master Oct 27, 2018

steveej deleted the return-future-without-result-and-work-examples branch October 27, 2018 12:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API: substitute `Result<Future..>` with `Future..` for more idiomatic future chaining #49

API: substitute `Result<Future..>` with `Future..` for more idiomatic future chaining #49

steveej commented Oct 17, 2018 •

edited

Loading

steveej Oct 17, 2018

steveej Oct 17, 2018

lucab commented Oct 17, 2018

steveej commented Oct 23, 2018

lucab Oct 24, 2018

lucab Oct 24, 2018

lucab Oct 24, 2018

lucab Oct 24, 2018

lucab Oct 24, 2018

lucab Oct 24, 2018

steveej Oct 24, 2018 •

edited

Loading

lucab Oct 24, 2018

lucab Oct 24, 2018

lucab Oct 24, 2018

lucab Oct 24, 2018

lucab Oct 24, 2018

lucab Oct 24, 2018

lucab commented Oct 24, 2018

steveej commented Oct 25, 2018

lucab left a comment

API: substitute Result<Future..> with Future.. for more idiomatic future chaining #49

API: substitute Result<Future..> with Future.. for more idiomatic future chaining #49

Conversation

steveej commented Oct 17, 2018 • edited Loading

Choose a reason for hiding this comment

FIXME

Choose a reason for hiding this comment

FIXME

lucab commented Oct 17, 2018

steveej commented Oct 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveej Oct 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucab commented Oct 24, 2018

steveej commented Oct 25, 2018

lucab left a comment

Choose a reason for hiding this comment

API: substitute `Result<Future..>` with `Future..` for more idiomatic future chaining #49

API: substitute `Result<Future..>` with `Future..` for more idiomatic future chaining #49

steveej commented Oct 17, 2018 •

edited

Loading

steveej Oct 24, 2018 •

edited

Loading