Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
Emilgardis opened this issue May 30, 2023 · 4 comments
Labels
enhancement New feature or request

Comments

@Emilgardis
Copy link
Member

Emilgardis commented May 30, 2023

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 #114 (comment) where we would make the allocation optional

@Emilgardis
Copy link
Member Author

Emilgardis commented May 30, 2023

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
Copy link

laundmo commented May 30, 2023

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 Emilgardis added the enhancement New feature or request label May 30, 2023
@Emilgardis
Copy link
Member Author

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

@Emilgardis
Copy link
Member Author

Emilgardis commented Nov 5, 2023

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`
            }
        }
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants