edgedb / edgedb-rust

The official Rust binding for EdgeDB
https://edgedb.com
Apache License 2.0
209 stars 26 forks source link

feat: accept `impl AsRef<str>` for query #314

Closed MrFoxPro closed 2 months ago

msullivan commented 2 months ago

This will result in extra allocations and copies though, right? (Not sure if we care.)

MrFoxPro commented 2 months ago

This will result in extra allocations and copies though, right? (Not sure if we care.)

Well, I'm not sure honestly. @aljazerzen do you have thought on this?

aljazerzen commented 2 months ago

Yes this will clone:

let a: &str = "hello";
let b = Into<String>::into(a); # this is a clone

You probably want AsRef<str>, which is reads as "trait that a type is able to turn into a references to a str".

aljazerzen commented 2 months ago

Although I'm not sure about this change. Such "input helper" generics are usually added on small functions, since these generics (might) generate different implementations of the function in the final binary.

May I ask, what's the type that you want to pass in? Does it not have .as_str() method?

MrFoxPro commented 2 months ago

Although I'm not sure about this change. Such "input helper" generics are usually added on small functions, since these generics (might) generate different implementations of the function in the final binary.

May I ask, what's the type that you want to pass in? Does it not have .as_str() method?

Just for passing String directly

aljazerzen commented 2 months ago

Well, for that you can just do:

let q: String = "select 1".to_string();

client.query(&q); # <--- this will convert it it into &str

We don't want to be taking String, because that would take over the ownership of the query, even though that is not really needed.

MrFoxPro commented 2 months ago

Well, for that you can just do:

let q: String = "select 1".to_string();

client.query(&q); # <--- this will convert it it into &str

We don't want to be taking String, because that would take over the ownership of the query, even though that is not really needed.

I think most of queries do not need to be owned by something other than client. I'm just lazy to write & every time. Especially at the start of macro, it's kinda uglier :p

aljazerzen commented 2 months ago

Well, you might want to pass the query to client multiple times and you cannot do that without cloning if you pass in the ownership.

In summary, I cannot approve Into<String>, but I would reluctantly approve AsRef<str>, which would allow you not to write the &.

It does add a bit to the complexity of our codebase and it might make final binaries bigger is some cases.