mediocregopher / radix.v2

Redis client for Go
http://godoc.org/github.com/mediocregopher/radix.v2
MIT License
433 stars 92 forks source link

r.Int64() not properly working #7

Closed daemonfire300 closed 9 years ago

daemonfire300 commented 9 years ago

Hi there, https://github.com/mediocregopher/radix.v2/blob/master/redis/resp.go#L310-L319 does not work, it says "could not convert to int". I wonder how this happens, because I though radix.v2 was the improved radix version. However the "old" version does not have this problem https://github.com/fzzy/radix/blob/master/redis/reply.go#L93-L115

elithrar commented 9 years ago

Can you post your code/an example that demonstrates it? On Fri, 4 Sep 2015 at 7:11 am Julius F notifications@github.com wrote:

Hi there,

https://github.com/mediocregopher/radix.v2/blob/master/redis/resp.go#L310-L319 does not work, it says "could not convert to int". I wonder how this happens, because I though radix.v2 was the improved radix version. However the "old" version does not have this problem https://github.com/fzzy/radix/blob/master/redis/reply.go#L93-L115

— Reply to this email directly or view it on GitHub https://github.com/mediocregopher/radix.v2/issues/7.

daemonfire300 commented 9 years ago

Preparing redis

SET samplekey 1234
> OK
GET samplekey
> "1234"

Now fetching this with

r := client.Get("samplekey")
val, err := r.Int64()

throws the error. And it is obvious why this happens. Redis returns strings, and the methods Int64 and Int do not attempt to parse the response value as such and simply return r.val.(Int). r.Int() and r.Int64() are effectively broken.

I will propose a fix tomorrow. Basically just checking the r.type and if it is redis.Str or redis.SimpleStr call strconv.ParseInt(r.val, 10, 64)..

mediocregopher commented 9 years ago

I'm sorry it's taken so long to respond, for some reason github decided I shouldn't receive notifications for issues on the repo >_<

You're correct about the cause of the issue. I'm not sure if I did this on purpose or not, but whatever my reasons I agree the behavior from the original radix made more sense. I'll implement a fix today.

daemonfire300 commented 9 years ago

@mediocregopher I hope my "critique" is not received in the wrong way. It would be perfectly ok if you decide as author to say, "this is intended, the user should handle possible parsing errors", but coming from radix "v1" it was simply an unexpected behaviour for me.

mediocregopher commented 9 years ago

No worries, I agree that it's unexpected, probably just a brain-fart on my part when I wrote that, and I guess I haven't used those methods all that much for it to have come up before. It's a backwards compatible change that I don't mind making.