Siriusmart / invidious-rs

Rust bindings for the Invidious API
GNU Affero General Public License v3.0
11 stars 5 forks source link

ClientAsync is not Clone #3

Closed DaKnig closed 1 year ago

DaKnig commented 1 year ago

Despite looking like ClientSync, it is not Clone.

Siriusmart commented 1 year ago

Yes, that's because ClientAsync contains MethodAsync, which can contains a custom method (Box<dyn Fn(String) -> Pin<Box<dyn Future<Output = MethodReturn> + Send>> + Send + Sync>) I couldn't make it clone.

So either I figure out how to make that clone, or maybe I will include an ClientAsyncClone struct which does not allow custom method.


Also, just by curiosity, when would you ever need to clone the client? Wouldn't it be a better practice to just put it in a OnceCell and use it from there?

DaKnig commented 1 year ago

consider the following code:

            let vid_data =
                self.invidious_client.borrow().video(&vid_id, None).await;

from my repo. in my case self.invidious_client is a RefCell<ClientAsync>, and this code invokes the clippy msg "borrow across await!" or so. in the future I want to add the ability to change this property on the fly, from settings or maybe dynamically choose the instance or something. it would make sense to clone this object (not that expensive) and then await that

DaKnig commented 1 year ago

Yes, that's because ClientAsync contains MethodAsync, which can contains a custom method (Box<dyn Fn(String) -> Pin<Box<dyn Future<Output = MethodReturn> + Send>> + Send + Sync>) I couldn't make it clone.

So either I figure out how to make that clone, or maybe I will include an ClientAsyncClone struct which does not allow custom method. how about making it a trait with a few implementers? a invidious::MethodAsync::Preset would implement it, but also Custom, then the first would be Clone and the second would not. the trait would not need to require Clone...

Siriusmart commented 1 year ago

So you would suggest doing something like

It's not that hard, I can do that.

Siriusmart commented 1 year ago

I've done it, ClientAsync is now Clone.

Bump the version up 0.6.0->0.6.1, unless you are using custom methods the code should just work.

Hope that helps.

Siriusmart commented 1 year ago

Using traits allows clients to be able to have custom fetch methods and be able to clone, which is really cool.

Also added ClientAsyncClone as a subset of ClientAsyncTrait

DaKnig commented 1 year ago

not sure how useful ClientAsyncClone is, its just ClientAsyncTrait + Clone, which would be more readable