Siriusmart / invidious-rs

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

No documentation for the meaning of "continuation" field in "Comments" #4

Closed DaKnig closed 1 year ago

DaKnig commented 1 year ago

most fields of most structs are easy to understand, this one is not, please explain what this field does and how to use it!

Siriusmart commented 1 year ago

In Invidious docs, it says that continuation is a parameter for comments, looks like it's a key for accessing the next page of comment (kinda like a linked list).

Hope that's correct.

DaKnig commented 1 year ago

maybe you should , as the API writer, encapsulate that behavior into a few functions? like, give me as a user a nice wrapper of said "linked list"? or at least some functions to deal with that?

Siriusmart commented 1 year ago

Ok, so if I go to something like https://vid.puffyan.us/api/v1/comments/h55emgImrLk, and the continuation string is EkMSC2g1NWVtZ0ltckxryAEA4.... To access the next page you will need to send a request to https://vid.puffyan.us/api/v1/comments/h55emgImrLk?continuation=EkMSC2g1NWVtZ0ltckxryAEA4....

Currently, you go like

let page1 = client.comments("h55emgImrLk", None)?;
let page2 = client.comments("h55emgImrLk", Some(&format!("continuation={}", page1.continuation.unwrap())?;

By "some functions" are you thinking about this?

let page2 = page1.next().unwrap();

One downside of doing so is that the structs will also need to store a clone of Client (it currently doesn't), is that what you're thinking about? I can turn the entire thing into a high level crate and feature gate it to make it optional if you think that'll make it better.

DaKnig commented 1 year ago

how about having an object "comments" that has both a vector of comments and a continuation? shove that wherever you have comments. then add a few convenience functions, for example I think this could be cool:

impl Comments {
    /// in place appending more messages
    async fn load_more_async<Client>(&mut self, &client: Client) where Client: ClientAsync;
    fn load_more_sync<Client>(...)
}

I think the second looks cleaner... but in any case, as an API user , I would not wanna mess with strings and requests directly, thats what the lib is for I think :)

DaKnig commented 1 year ago

changing the vector in place could be cool. or returning another page, that can also be cool. whichever you choose, I would love it.

DaKnig commented 1 year ago

hi, any update on this? please dont close the issue until you either solve it or it becomes a wontfix

Siriusmart commented 1 year ago

currently not doing it, but planning to

Siriusmart commented 1 year ago

update: I tried, but failed.

The problem with using Arc<C> where C: ClientSyncTrait is that I will need to change the trait PublicItem (which is a trait implmented for all binding structs) to include a generic PublicItem<C>, as a result when implementing this trait for the bindings structs, not only will I have to impl say PublicItem<C: ClientSync>, it also forces me to impl it for everything else which impls ClientSyncTrait, therefore it will not work with all custom clients, an arguably more important feature. As a result, this approach to creating a high level wrapper will not work.

The other way I tried to do it is to store it as an Arc<Box<dyn ClientSyncTrait>> in the binding structs, this removes the need for generics which seem to be causing a lot of issues. However, the problem with this is that ClientSync::new() -> Self requires Self to impl Sized, which prevents the struct from being made into a trait object, therefore this approach wouldn't work either.


I've spent quite a sizeable amount of time on this, so unless there are other ways to store the client in the binding structs, or a way to create a high level wrapper without the need of storing the client within the struct, I would like to know. But before that happens I think I'm going to give up on this upgrade and leave it as a lower level binding.

Siriusmart commented 1 year ago

failed.tar.gz

here is what I got so far before I gave up, so if anyone think I was going in the right direction, you can pick it up and make it so that the client is stored in the binding struct, but I don't think that's the right approach.

Siriusmart commented 1 year ago

shit i accidentally closed it as completed

DaKnig commented 1 year ago

you dont need to store anything, ask for it as an argument to method

Siriusmart commented 1 year ago

if you are already doing

let channel = video.channel(client);

why not just

let channel = client.channel(video.channel.id, None);

tbh it barely makes a difference

Siriusmart commented 1 year ago

i see the crate doing just fine with its own consistent way of fetching, and i'll just leave it as-is