Giter VIP home page Giter VIP logo

Comments (10)

Emilgardis avatar Emilgardis commented on June 6, 2024 1

this looks promising for the deserialization: yoke and zerofrom

from twitch_api.

laundmo avatar laundmo commented on June 6, 2024 1

thank you, that .map(Into::into) worked and i so much nicer (so nice in fact, that i dont think a custom (to avoid orphan rules) conversion trait would be useful)

i'll probably send you a message on discord in a bit with some smaller things tho after looking through the issues, you seem to be aware of most of them (like the slices are already discussed herem, and the builders not being ideal).

from twitch_api.

Emilgardis avatar Emilgardis commented on June 6, 2024

https://docs.rs/aliri_braid seems like a good way to solve this!

from twitch_api.

Nerixyz avatar Nerixyz commented on June 6, 2024

I tried to "convert" the GetUsersRequest as a POC to use reference types (see diffs below). Using references in the struct has the consequence, that we can't pass a "static"/immediate reference to a slice, but need to create a variable for the array first:

let req = GetUsersRequest::builder().id(&["44322889".into()]).build();

This will result in the following error:

error[E0716]: temporary value dropped while borrowed
   --> src\helix\endpoints\users\get_users.rs:112:46
    |
112 |     let req = GetUsersRequest::builder().id(&["44322889".into()]).build();
    |                                              ^^^^^^^^^^^^^^^^^^^         - temporary value is freed at the end of this statement
    |                                              |
    |                                              creates a temporary which is freed while still in use
...
139 |     let uri = req.get_uri().unwrap();
    |               ------------- borrow later used here
    |
help: consider using a `let` binding to create a longer lived value
    |
112 ~     let binding = ["44322889".into()];
113 ~     let req = GetUsersRequest::builder().id(&binding).build();
    |

Fix:

let ids = ["44322889".into()];
let req = GetUsersRequest::builder().id(&ids).build();

Diffs:

helix/endpoints/users/get_users.rs
diff --git a/src/helix/endpoints/users/get_users.rs b/src/helix/endpoints/users/get_users.rs
index 49333d8d3..0f5789471 100644
--- a/src/helix/endpoints/users/get_users.rs
+++ b/src/helix/endpoints/users/get_users.rs
@@ -25,9 +25,11 @@
 //! # let client: helix::HelixClient<'static, client::DummyHttpClient> = helix::HelixClient::default();
 //! # let token = twitch_oauth2::AccessToken::new("validtoken".to_string());
 //! # let token = twitch_oauth2::UserToken::from_existing(&client, token, None, None).await?;
+//! let user_ids = ["1234".into()];
+//! let usernames = ["justintvfan".into()];
 //! let request = get_users::GetUsersRequest::builder()
-//!     .id(vec!["1234".into()])
-//!     .login(vec!["justintvfan".into()])
+//!     .id(&user_ids)
+//!     .login(&usernames)
 //!     .build();
 //! let response: Vec<get_users::User> = client.req_get(request, &token).await?.data;
 //! # Ok(())
@@ -43,15 +45,15 @@ use helix::RequestGet;
 /// Query Parameters for [Get Users](super::get_users)
 ///
 /// [`get-users`](https://dev.twitch.tv/docs/api/reference#get-users)
-#[derive(PartialEq, typed_builder::TypedBuilder, Deserialize, Serialize, Clone, Debug)]
+#[derive(PartialEq, typed_builder::TypedBuilder, Serialize, Clone, Debug)]
 #[non_exhaustive]
-pub struct GetUsersRequest {
+pub struct GetUsersRequest<'a> {
     /// User ID. Multiple user IDs can be specified. Limit: 100.
     #[builder(default)]
-    pub id: Vec<types::UserId>,
+    pub id: &'a [&'a types::UserIdRef],
     /// User login name. Multiple login names can be specified. Limit: 100.
     #[builder(default)]
-    pub login: Vec<types::UserName>,
+    pub login: &'a [&'a types::UserNameRef],
 }
 
 /// Return Values for [Get Users](super::get_users)
@@ -91,7 +93,7 @@ pub struct User {
     pub view_count: usize,
 }
 
-impl Request for GetUsersRequest {
+impl<'a> Request for GetUsersRequest<'a> {
     type Response = Vec<User>;
 
     #[cfg(feature = "twitch_oauth2")]
@@ -101,15 +103,14 @@ impl Request for GetUsersRequest {
     const SCOPE: &'static [twitch_oauth2::Scope] = &[];
 }
 
-impl RequestGet for GetUsersRequest {}
+impl<'a> RequestGet for GetUsersRequest<'a> {}
 
 #[cfg(test)]
 #[test]
 fn test_request() {
     use helix::*;
-    let req = GetUsersRequest::builder()
-        .id(vec!["44322889".into()])
-        .build();
+    let ids = ["44322889".into()];
+    let req = GetUsersRequest::builder().id(&ids).build();
 
     // From twitch docs
     // FIXME: This is not valid anymore. Twitch....
helix/client/client_ext.rs
diff --git a/src/helix/client/client_ext.rs b/src/helix/client/client_ext.rs
index 4d70b442c..4d963b6d1 100644
--- a/src/helix/client/client_ext.rs
+++ b/src/helix/client/client_ext.rs
@@ -12,7 +12,7 @@ impl<'a, C: crate::HttpClient<'a> + Sync> HelixClient<'a, C> {
     /// Get [User](helix::users::User) from user login
     pub async fn get_user_from_login<T>(
         &'a self,
-        login: impl Into<types::UserName>,
+        login: impl AsRef<types::UserNameRef>,
         token: &T,
     ) -> Result<Option<helix::users::User>, ClientError<'a, C>>
     where
@@ -20,7 +20,7 @@ impl<'a, C: crate::HttpClient<'a> + Sync> HelixClient<'a, C> {
     {
         self.req_get(
             helix::users::GetUsersRequest::builder()
-                .login(vec![login.into()])
+                .login(&[login.as_ref()])
                 .build(),
             token,
         )
@@ -31,7 +31,7 @@ impl<'a, C: crate::HttpClient<'a> + Sync> HelixClient<'a, C> {
     /// Get [User](helix::users::User) from user id
     pub async fn get_user_from_id<T>(
         &'a self,
-        id: impl Into<types::UserId>,
+        id: impl AsRef<types::UserIdRef>,
         token: &T,
     ) -> Result<Option<helix::users::User>, ClientError<'a, C>>
     where
@@ -39,7 +39,7 @@ impl<'a, C: crate::HttpClient<'a> + Sync> HelixClient<'a, C> {
     {
         self.req_get(
             helix::users::GetUsersRequest::builder()
-                .id(vec![id.into()])
+                .id(&[id.as_ref()])
                 .build(),
             token,
         )
@@ -50,7 +50,7 @@ impl<'a, C: crate::HttpClient<'a> + Sync> HelixClient<'a, C> {
     /// Get [ChannelInformation](helix::channels::ChannelInformation) from a broadcasters login
     pub async fn get_channel_from_login<T>(
         &'a self,
-        login: impl Into<types::UserName>,
+        login: impl AsRef<types::UserNameRef>,
         token: &T,
     ) -> Result<Option<helix::channels::ChannelInformation>, ClientError<'a, C>>
     where
@@ -66,7 +66,7 @@ impl<'a, C: crate::HttpClient<'a> + Sync> HelixClient<'a, C> {
     /// Get [ChannelInformation](helix::channels::ChannelInformation) from a broadcasters id
     pub async fn get_channel_from_id<T>(
         &'a self,
-        id: impl Into<types::UserId>,
+        id: impl AsRef<types::UserIdRef>,
         token: &T,
     ) -> Result<Option<helix::channels::ChannelInformation>, ClientError<'a, C>>
     where
@@ -74,7 +74,7 @@ impl<'a, C: crate::HttpClient<'a> + Sync> HelixClient<'a, C> {
     {
         self.req_get(
             helix::channels::GetChannelInformationRequest::builder()
-                .broadcaster_id(id.into())
+                .broadcaster_id(id.as_ref())
                 .build(),
             token,
         )
@@ -361,7 +361,7 @@ impl<'a, C: crate::HttpClient<'a> + Sync> HelixClient<'a, C> {
     /// Get a users, with login, follow count
     pub async fn get_total_followers_from_login<T>(
         &'a self,
-        login: impl Into<types::UserName>,
+        login: impl AsRef<types::UserNameRef>,
         token: &T,
     ) -> Result<Option<i64>, ClientError<'a, C>>
     where
@@ -547,7 +547,7 @@ impl<'a, C: crate::HttpClient<'a> + Sync> HelixClient<'a, C> {
     /// Get channel emotes in channel with user login
     pub async fn get_channel_emotes_from_login<T>(
         &'a self,
-        login: impl Into<types::UserName>,
+        login: impl AsRef<types::UserNameRef>,
         token: &T,
     ) -> Result<Option<Vec<helix::chat::ChannelEmote>>, ClientError<'a, C>>
     where

from twitch_api.

Emilgardis avatar Emilgardis commented on June 6, 2024

Should investigate if there is a better surface for storing multiple values than

foos: Cow<'a, [&'a FooRef]>,

the issue is that requiring FooRef means that there is a situation where your collection is Vec<Foo> and to actually pass it to the request you'd need to do let v: Vec<&'a FooRef> = vec.iter().map(Deref::deref).collect() which is another allocation.

It'd be neat to abstract this so that all variants works. i.e passing &[Foo], [Foo; _], Vec<Foo>, Vec<&FooRef>, &[&FooRef].

The trait that unifies these should be IntoIterator<Item = Into<&FooRef>>, however it's not very nice. This is what I came up with

use std::borrow::Cow;
use twitch_types as types;

fn main() {
    let ids: Vec<types::UserId> = vec!["123".into(), "456".into(), "789".into()];
    println!("{:p}", &*ids);

    let req = Request {
        id: Cow::from(&ids),
        _pd: <_>::default(),
    };
    println!("{:p}", &*req.id);
}

pub struct Request<'a, 'i, IT, D: 'a>
where
    &'i IT: IntoIterator<Item = D>,
    IT: ToOwned + ?Sized,
    D: ?Sized + Into<&'a types::UserIdRef>,
{
    pub id: Cow<'i, IT>,
    _pd: std::marker::PhantomData<&'a ()>,
}
    Finished dev [unoptimized + debuginfo] target(s) in 0.36s
     Running `target/debug/twitch_api_ref_coll`
0x1516069e0
0x1516069e0

from twitch_api.

Emilgardis avatar Emilgardis commented on June 6, 2024

Maybe an enum

enum Cowtainer<'a, T> where T: ToOwned + ?Sized {
    One(Cow<'a, T>),
    Multiple(Cow<'a, [&'a T]>),
}

from twitch_api.

Emilgardis avatar Emilgardis commented on June 6, 2024

Lessons from ICU4X (with great talk https://www.youtube.com/watch?v=DM2DI3ZI_BQ)

VarZeroVec basically implements Cow<'a, [&'a T]> but nicer (depending on how you see it)

I'm still not satisfied though, I'll experiment with this

from twitch_api.

laundmo avatar laundmo commented on June 6, 2024

as a new user of this crate, the usage of braid and Cow in many places is not something i have encountered in any other crate and leads to quite verbose code. for example, going from a Option<Cursor> to a Option<Cow<CursorRef>> requires this monstrosity: input.as_ref().map(|c| Cow::Borrowed(c.as_ref()))

that is simply not a nice API. i hope it can be improved in some way.

personally i would be perfectly fine with just using String everywhere - the few extra allocations are a price i would happily pay to be able to simplify my code a lot.

from twitch_api.

laundmo avatar laundmo commented on June 6, 2024

if there is no willingness to drop braid, i imaging a From implementation could be used:

impl From<Option<Cursor>> for Option<CursorRef> { /*...*/}

from twitch_api.

Emilgardis avatar Emilgardis commented on June 6, 2024

@laundmo Happy to have you here! Thank you for the feedback, there is definitely a trade-off we need to take into consideration with this.

As for the Cursor problem the following should work fine and is much smaller: cursor.map(Into::into). Using into should be enough thanks to this impl

fn cow<'a>(cursor: Option<Cursor>) -> Option<std::borrow::Cow<'a, CursorRef>> {
    cursor.map(Into::into)
}

The suggestion wouldn't work with that signature for the same reason there's no impl From<Option<String>> for Option<str>, it'd have to be for Option<&'a str> as Option<T> has an implicit T: Sized bound. There is no valid lifetime to pick here either so we're stuck. Further, this hits the, very unfortunate and annoying, orphan rule.

Option<CursorRef> is not a valid Option:

error[E0277]: the size for values of type `str` cannot be known at compilation time
   --> src/helix/mod.rs:168:39
    |
168 |     fn from(value: Option<Cursor>) -> Option<CursorRef> {
    |                                       ^^^^^^^^^^^^^^^^^ doesn't have a size known at compile-time
    |
    = help: within `helix::CursorRef`, the trait `std::marker::Sized` is not implemented for `str`
note: required because it appears within the type `CursorRef`
   --> src/helix/mod.rs:165:1
    |
165 | #[aliri_braid::braid(serde)]
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: required by a bound in `std::option::Option`
   --> /Users/emil/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/option.rs:563:17
    |
563 | pub enum Option<T> {
    |                 ^ required by this bound in `Option`
    = note: this error originates in the attribute macro `aliri_braid::braid` (in Nightly builds, run with -Z macro-backtrace for more info)

and the orphan rule:

impl From<Option<Cursor>> for Option<CursorRef> {
    fn from(value: Option<Cursor>) -> Self {
        value.map(Into::into)
    }
}

fails with

error[E0117]: only traits defined in the current crate can be implemented for types defined outside of the crate
   --> src/helix/mod.rs:168:1
    |
168 | impl From<Option<Cursor>> for Option<CursorRef> {
    | ^^^^^--------------------^^^^^-----------------
    | |    |                        |
    | |    |                        `std::option::Option` is not defined in the current crate
    | |    `std::option::Option` is not defined in the current crate
    | impl doesn't use only types from inside the current crate
    |
    = note: define and implement a trait or new type instead

this also hinders a impl From<Option<Cursor>> for Option<Cow<'static, CursorRef>>, std would have to implement it

regarding simplifying the code, if you have some examples I'd love to have a look to see if there's anything we can do in this codebase or clarify how to consume the api in a convenient way. feel free to drop them in a issue or discussion here or create a thread over at our channel #twitch-rs in Twitch API Discord (or just reach out to me directly)

from twitch_api.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.