FromResp implementation for more integers + boolean?

[benashford]

FromResp implementation for more integers + boolean?

[benashford]

Published in version 0.4.2

[benashford]

Hello,

Thanks for raising this. It is an interesting area as it highlights the gap between how Rust models the world vs. how Redis models the world. I think the goal should be to avoid surprises, but there's a few surprises already.

The reason why i64 is currently supported is because according to this: https://redis.io/topics/protocol all Redis integers are guaranteed to be within a signed 64-bit integer. However, usize is also supported just for sheer pragmatism even though it might be too small on 32-bit platforms. But this could be addressed by raising an error at runtime should this happen.

One other surprise is String will support integer by converting it to a string along the way, which it probably shouldn't as I don't think this helps (I can't actually remember why it does this).

There are some other pragmatic examples too, e.g. FromResp is supported on () which matches the string "OK" and nothing else.

So, in theory, yes, I think your proposal is a good idea. But there might be an issue with u8 as Vec<u8> has a specific implementation of FromResp to support raw bytes being read/written to Redis, which would clash with the Vec<T> implementation for reading RESP arrays. So maybe would be safest to implement just for u16, u32, u64, i16, i32, and i64 plus bool?

[daboross]

Ah, the protocol reasoning makes sense. Thank you for linking that!

I don't have any problem leaving out i8/u8.

In day-to-day data in rust I use i32 and u32 the most, i64 and u64 sometimes, and rarely isize and usize. Personally, I have no use case for 8-bit nor 16-bit types, and I was only including them for completion. If 8-bit types can't be included, I'd rather just implement it for the common integers i32, u32, i64 and u64.

I think either implementing it with or without 16-bit types would work well.

---

Side thought: maybe this should live in a higher level crate which builds on redis-async?

Having u8 conflict with Vec<u8> makes perfect sense for this crate- but I could imagine if someone built a higher level handler which knew about the various methods and what they returned, that higher level handler could differentiate between expecting a byte blob and a list of integers based on the command used.

---

In any case, I'm good with just supporting what we can support, and there doesn't seem harm in supporting i32, u32, u64 and bool. If that sounds good to you, I'll make a PR.

[benashford]

It certainly could live in a higher-level crate.

One lurking idea I've had for a while is that the send on PairedConnection should be built-upon to provide functions for each Redis command, so connection.set(key, value) rather than connection.send(resp_array!["SET", key, value]) at present. In this world each function could do the right thing to return the right result.

But after looking into it, I haven't done that yet as:

1. It would tie this crate to a particular version of Redis, although if it was implemented as a higher-level crate that could be less of an issue.

2. There are idiosyncrasies in how Redis commands have been implemented, which would mean implementing each by hand, only the simplest could be built from a macro. It's common in Redis clients for dynamic languages to generate the client automatically from the list of commands, but I don't think that would be possible for Rust.

But yes, I'd be happy to accept a PR that implemented FromResp for i32, u32, u64, and bool.

[daboross]

Alright. Not doing the higher-level interface in this crate seems reasonable.

I'll make a PR for those FromResp implementations in this crate- thanks!