fred.rs icon indicating copy to clipboard operation
fred.rs copied to clipboard

RedisKey::from(&str) constraint to 'static lifetimes

Open nviennot opened this issue 1 year ago • 1 comments

Typically, when callers pass a key of type &str, it most of the time from a &'static str. This pull request takes this into account and avoid an allocation.

If the caller has constructed a string, for example with format!("users/{}", user.id), then we'll take this as a String, and there's no allocation needed.

If the caller constructs a string, but wants to reuse it, the caller will have to invoke .clone() on the string.

nviennot avatar Jul 17 '22 22:07 nviennot

Hi @nviennot,

Thanks for the PR. A few things:

I'm not sure I agree with this:

Typically, when callers pass a key of type &str, it most of the time from a &'static str.

In my experience, especially with keys, these are almost never static. However, at the end of the day this is subjective and the library needs to support both use cases no matter what.

In an ideal world we could specialize the Into impl's so that 'static types went through a happy path to avoid allocations, but unfortunately that's not on the table. I opted to follow the same pattern the bytes crate uses with respect to from_static, which is to say that the "easy" path (via Into) is the most ergonomic, but not necessarily the most performant. For the callers that want the added performance of avoiding allocations, they can use the from_static variant on RedisKey. The bytes crate is far more popular than this library, so I'm also piggybacking on their design decisions here a bit too.

All things considered I think I'm going to hold off on this unless anybody can give a compelling argument for how this moves the needle on ergonomics or use cases with this library in general. At the end of the day it's possible to avoid allocations if needed with either approach, but this change would be a breaking change, so for that reason alone I'd like to hold off.

aembke avatar Jul 18 '22 22:07 aembke