ferristseng / rust-ipfs-api

IPFS HTTP client in Rust
Apache License 2.0
247 stars 68 forks source link

Expose requests module and add builder for requests like add_with_opt… #52

Closed ArdaXi closed 4 years ago

ArdaXi commented 4 years ago

This is obviously heavily based on #29, I left out some options in the API that wouldn't make sense through this library (recursive, progress). I went with derive_builder mainly because for a fully optional struct, we don't need checks anywhere and I'm not sure the unwrap is a massive deal (it could be abstracted away).

One thing that concerns me about this design is that this will result in a lot of _with_options methods which do mostly the same thing.

Unfortunately, the only alternative I can think of would be to add a send(self) -> *Response method on each ApiRequest which would do what the request methods on the client do now, with its own downsides: it's a major change (though it could be hidden from the API) and it would necessary expose the client.request* methods (could be pub(crate)). I guess it could also be send(self, FnOnce(self) -> T) -> T if you want to keep the code in the client and support things like add_path nicely too, but that kinda sounds even crazier.

Either would however remove the distinction between, say, add and add_with_options, replacing them with something like client.add(data).send() and client.add(data).pin(false).send() respectively.

As it stands I feel like I'm massively overthinking this so I'm just sending this in and if this is acceptable I can extend this to all the methods that accept arguments.

ferristseng commented 4 years ago

This looks great @ArdaXi! Thank you for taking the time to work on this!

One thing that concerns me about this design is that this will result in a lot of _with_options methods which do mostly the same thing.

I'm also slightly concerned by the size of IpfsClient. I've been working on trying to refactor code away from it where possible (mostly been targeting internal methods right now). That being said, I am hesitant to shift the API too much in a different direction right now just due to time constraints on my end.

Unfortunately, the only alternative I can think of would be to add a send(self) -> Response method on each ApiRequest which would do what the request methods on the client do now, with its own downsides: it's a major change (though it could be hidden from the API) and it would necessary expose the client.request methods (could be pub(crate)). I guess it could also be send(self, FnOnce(self) -> T) -> T if you want to keep the code in the client and support things like add_path nicely too, but that kinda sounds even crazier.

I'd definitely be interested to see what this looks like, if it's not too much work to do. I think the send(self, FnOnce(self) -> T) -> T is a good, minimal way to see if there's a wider interest in moving the API in that direction.

As it stands I feel like I'm massively overthinking this so I'm just sending this in and if this is acceptable I can extend this to all the methods that accept arguments.

Thanks for being so thorough with this! What you have looks great for now, and I'm down to add an experimental API in a follow up PR!