Emilgardis
commented
3 years ago https://docs.rs/aliri_braid seems like a good way to solve this!
Open Emilgardis opened 3 years ago
Emilgardis
commented
3 years ago https://docs.rs/aliri_braid seems like a good way to solve this!
Nerixyz
commented
2 years ago 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:
Emilgardis
commented
2 years ago this looks promising for the deserialization: yoke and zerofrom
Emilgardis
commented
1 year ago 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
0x1516069e0Maybe an enum
enum Cowtainer<'a, T> where T: ToOwned + ?Sized {
One(Cow<'a, T>),
Multiple(Cow<'a, [&'a T]>),
}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
laundmo
commented
1 year ago 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.
laundmo
commented
1 year ago if there is no willingness to drop braid, i imaging a From implementation could be used:
impl From<Option<Cursor>> for Option<CursorRef> { /*...*/}@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 insteadthis 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)
laundmo
commented
1 year ago 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).
Started work on making everything use references instead of allocating on multiple places
https://github.com/twitch-rs/twitch_api/tree/references
This solution should work, but I'm not sure the approach is good.
I'd want requests to be able to take references, and responses return them too.
Suggestions for how to avoid unnecessary allocations in responses hugely appreciated!