twitch-rs / twitch_api

Rust library for talking with the Twitch API aka. "Helix", TMI and more! Use Twitch endpoints fearlessly!
Apache License 2.0
150 stars 33 forks source link

Clarify how or make it easier to pass in collections that are T: Iterator<Item=D> in builder functions for helix requests #374

Closed Emilgardis closed 1 month ago

Emilgardis commented 1 year ago

This came up in discussion over at Twitch API Discord

currently, it can be hard to reason how to deal with a method like

fn(Self, impl Into<Cow<'a, [&'a Borrowed]>>)

we have a bunch of these after #280

One way to deal with this, given we have a Vec<String>, is

fn builder(s: impl Into<Cow<'a, [&'a Borrowed]>>) -> Foo {
    todo!()
}

fn bar(items: Vec<String>) {
    let items: Vec<&Borrowed> = items.iter().map(Into::into).collect();
    builder(items)
}

but coming up with this is not apparent.

This is related to https://github.com/twitch-rs/twitch_api/issues/114#issuecomment-1323517268 where we would make the allocation optional

Emilgardis commented 1 year ago

We could clarify in the documentation for the methods how to do this, or make new methods that take impl Iterator<Item = impl Into<&'a Borrowed>> which are then collected and put in a Cow::Owned.

I think I prefer the second alternative, since one would have to do an allocation either way if you have a Vec<Owned>.

@laundmo also mentions in the discord thread a solution that would involve a impl CollectionRef which abstracts over a collection, this would work for the impl Iterator route, and it would also work for a route where we make the stored collection in the Request generic over collection for those fields, albeit at a cost of more generics on that specific request.

laundmo commented 1 year ago

heres my CollectionRef idea: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=14019aa8f991b7ec5eef82f29879a459

a great advantage is that this works on many collections and even individual items

Emilgardis commented 1 year ago

This could also be applied for the client ext methods which currently takes impl AsRef<[&'client types::CategoryIdRef]>

Emilgardis commented 1 year ago

also experimented with

fn into_slice_cow(self) -> std::borrow::Cow<'a, [R]>
    where
        [R]: ToOwned,
        R: Sized + 'a, {
        match self {
            std::borrow::Cow::Borrowed(b) => {
                std::borrow::Cow::Borrowed(std::slice::from_ref(b.into()))
            }
            std::borrow::Cow::Owned(o) => {
                let a: std::borrow::Cow<'_, R> = std::borrow::Cow::Owned(o.into());

                let a = std::slice::from_ref(a.as_ref());

                std::borrow::Cow::Borrowed(a) // ~ cannot return value referencing local variable `a`
            }
        }
    }
Emilgardis commented 1 month ago

improved with #359