Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consistency and documentation of return value processing #36

Open
Grinnz opened this issue Jan 29, 2019 · 6 comments
Open

Consistency and documentation of return value processing #36

Grinnz opened this issue Jan 29, 2019 · 6 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@Grinnz
Copy link
Collaborator

Grinnz commented Jan 29, 2019

The following commands have return values that can possibly map to nicer structures in Perl:

bzpopmax, bzpopmin - empty array, or array of [key, score, value], nonblocking only
blpop, brpop - empty array, or array of [key, value], nonblocking only
command - array of undefs and [name, arity, [flags array], first key position, last key position, key repeat step] arrays
geopos - array of undefs and [long, lat] arrays
hgetall - array of (field, value) pairs
time - array of [seconds, microseconds]
xrange, xrevrange - array of [id, [array of ordered (field, value) pairs]] arrays
xread, xreadgroup - array of [key, [id, [array of ordered (field, value) pairs]]] arrays
zpopmax, zpopmin - array of ordered (score, value) pairs
zrange, zrangebyscore, zrevrange, zrevrangebyscore - array of values, or with WITHSCORES option, ordered (value, score) pairs

Currently blpop, brpop, geopos, and hgetall have special processing on the return value, but this is not documented for hgetall. xread has an xread_structured option that returns a hash by key but the same array structure as the value. Of note, the (field, value) pairs for streams (x*) are ordered, so these should not be mapped to hashes, so there is probably no better structure for these stream commands. Pairs from the sorted set (z*) commands are also ordered. Trying to account for the WITHSCORES option or other options that change output formats is probably also not worthwhile.

That leaves bzpopmax, bzpopmin, time, and possibly command, and xreadgroup could have a structured variant.

@jhthorsen jhthorsen added enhancement New feature or request help wanted Extra attention is needed labels Jan 30, 2019
@jhthorsen
Copy link
Owner

@Grinnz: I'm a bit confused about what you're suggesting: The list after "...map to nicer structures in Perl" is that what the methods in Mojo::Redis::Database returns today, what you want them to return or what the actual Redis server returns?

Could you make an suggestion for how you want the return value from bzpopmax, bzpopmin, time should look?

@kraih: I think I'll remove the experimental flag after we've settled this issue. So if you are not going to use this commands, then you're good - unless you or others have feedback about the actual API.

@Grinnz
Copy link
Collaborator Author

Grinnz commented Jan 31, 2019

The list is the format that redis returns, I haven't made any suggestion as to what they should look like in the API. I would suggest bzpopmax and bzpopmin should similarly to blpop and brpop be extracted to a list to be passed to the callback, but I'm not sure the reordering is a good idea for any of those cases. The key name returned first is important when you run it on multiple lists or sorted sets, and any reordering increases the surprise factor for those reading the redis docs.

For time, maybe it could be concatenated into a single string similar to Time::HiRes::time?

@jhthorsen
Copy link
Owner

I will change "time" now. I need to think about the others for a bit.

I think I also need to change back to not returning a list, since it's in conflict with my idea of just passing in one argument to a promise.

@jhthorsen
Copy link
Owner

I also wonder if I should revert the change about the order of values from blpop and friends. Might make it less surprising to use.

These are important questions that need an answer before I remove EXPERIMENTAL.

@Grinnz
Copy link
Collaborator Author

Grinnz commented Jan 31, 2019

Unfortunately I don't use any of those commands so I can't say from experience what would be better. But I lean toward less surprising.

@mohawk2
Copy link
Contributor

mohawk2 commented May 9, 2020

Belatedly, and from the outside, I think "less surprising" would be the wise option!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants