camallo / dkregistry-rs

A pure-Rust asynchronous library for Docker Registry API v2
Apache License 2.0
62 stars 39 forks source link

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

Closed steveej closed 5 years ago

steveej commented 6 years ago

This change is motivated by an out-of-band discussion with @lucab and a comment on a related PR https://github.com/openshift/cincinnati/pull/4#discussion_r225639618 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.


lucab commented 6 years ago

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 commented 5 years ago

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

lucab commented 5 years ago

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

steveej commented 5 years ago

Yay, more unwraps to clear up :birthday: Thanks for catching these