#500: Added JSON.RESP Command #751
Conversation
|
@JyotinderSingh - Please review this PR. I closed the old PR due to some issues I was facing merging with master. You had left 4 comments there. 3 of them are addressed in 3 commits above. 1 was a question on type switch for which I have left a comment there. |
There was a problem hiding this comment.
Thanks for adding support for the JSON.RESP command, @psrvere! The changes mostly look good, had a few comments.
| intValue, isInteger := utils.IsFloatToIntPossible(v.(float64)) | ||
| if isInteger { | ||
| return []byte(fmt.Sprintf(":%d\r\n", intValue)) | ||
| } | ||
|
|
||
| // if it is a float, encode like a string | ||
| str := strconv.FormatFloat(v.(float64), 'f', -1, 64) | ||
| return encodeString(str) |
There was a problem hiding this comment.
Are we sure this modified logic doesn't end up messing up cases where someone stores a whole number in floating point representation? Something like 5.0? How does redis behave in such cases?
There was a problem hiding this comment.
A whole number in floating point representation is shown as string in Redis and Dice master branch.
On Dice master branch:
set a 1
OK
get a
(integer) 1
set a 1.34
OK
get a
"1.34"
set a 1.00
OK
get a
"1.00"
The behaviour of JSONResp feature branch is identical as master branch for above cases.
On Redis:
set a 1
OK
get a
"1" ---- Integers are also encoded as strings in Redis (different behaviour)
set a 1.34
OK
get a
"1.34"
set a 1.00
OK
get a
"1.00"
I am also working on #69. Let me take up integer difference there.
I did run all unit tests for eval_test.go and all integration tests and they all passed.
|
Please resolve conflicts. |
|
@JyotinderSingh - addressed all comments. Please review. |
Old PR - #631 (closed)
This PR fixes #500
The PR
resp.goto support boolean and integer encoding, and adds unit tests for the sameIsFloatToIntPossible