Hash replies support

[aGrmbl]

It would be nice for reply.Val() to return map[string]string for hash queries to Redis (HGetAll() etc.) instead of []string. Would require new reply type it seems.

[vmihailenco]

I don't want to make backwards incompatible change without great need. So I would propose to add new method HGetAllMap. What other commands need similar change? I don't see other candidates.

[aGrmbl]

Looking at Redis docs it looks like HGETALL is the main one. :)

There're commands like ZRANGE / ZREVRANGE / ZREVRANGEBYSCORE / ZRANGEBYSCORE ... WITHSCORES which return map[string]int64.

P.S. Want to say "thank you" for your client, it's quite good. Владимир, большое Вам спасибо! :)

[vmihailenco]

It is currently in separate branch, but I will merge it soon if there is no issues with implementation:

5a90e32

What do you think?

PS Пожалуйста :)

[aGrmbl]

Whoa, that was fast. :) Looking at code it seems to be OK (changes are rather simple).

There's one thing i've noticed (not being golang expert in any way, so i might be wrong):

You're using type assertions on values returned from parseReply() which will return (nil, nil) for NULL Redis reply elements ("*-1\r\n").
Problem might be that type asserions will fail for nil values, which will result in returning error (but not nil value).

A bit of code to illustrate this:

package main

func crapit() interface{} {
    return nil
}

func main() {
    bad := crapit()

    if _, ok := bad.(string); !ok {
        panic("aieee")  // For Redis we would want to return nil if bad==nil
    }
}

[vmihailenco]

Redis nil values are returned as Golang redis.Nil: http://godoc.org/github.com/vmihailenco/redis#_variables:

https://github.com/vmihailenco/redis/blob/master/parser.go#L171

So parseReply should not return both value and err set to nil. I think we are safe here.

But in general I probably have to handle err == redis.Nil. But HMGETALL and ZRANGE* can't return nil values (at least I think so). Also I don't know know the best way to handle it right now, because I can' store nil in map[string]string and map[string]float64. I guess I will have to check for redis.Nil and translate it to more suitable error. Not sure. I prefer to not handle it right now until we have real use case.

[aGrmbl]

Makes perfect sense, thanks for you explanation. And yeah, NULL values are not that common, it's more of a theoretical case.

I probably was confused by https://github.com/vmihailenco/redis/blob/master/parser.go#L188 a bit. BTW, doc says that NULL multi-bulks can happen with BLPOP timing out, and NULL items can happen with SORT.

Anyway, thanks a lot for all your work, so far your client works quite good (switched from radix since that guy just nuked repository).

[vmihailenco]

Ah, I misunderstood your question. I was speaking about nil bulk reply ($-1).

With nil multi-bulk reply library will return res.Err() == redis.Nil, res.Val() will be nil. I added test for BLPOP:

ed0d065

There is no test for SORT, because I can'f find in the docs when SORT can return nil reply. Looks like it return reply with 0 results instead.

In general I try to handle redis.Nil values as good as possible. So please contact me if lib behaves badly.

[aGrmbl]

What i've found in Redis protocol spec (http://redis.io/topics/protocol): "Single elements of a multi bulk reply may have -1 length, in order to signal that this elements are missing and not empty strings. This can happen with the SORT command when used with the GET pattern option when the specified key is missing."

Again, it's more of a theoretical question, stuff i do with redis do not involve anything more complex than HIncrBy. :)

Thanks for your time.