Add RedisResult.AsBoolSlice() #499 #500
Conversation
message.go
Outdated
| if m.typ != '*' { | ||
| panic(fmt.Sprintf("redis message type %c is not an array", m.typ)) | ||
| } |
There was a problem hiding this comment.
| if m.typ != '*' { | |
| panic(fmt.Sprintf("redis message type %c is not an array", m.typ)) | |
| } |
This check is already done by m.ToArray()
message.go
Outdated
| if len(v.string) != 0 { | ||
| s[i], err = strconv.ParseBool(v.string) | ||
| if err != nil { | ||
| s[i] = false | ||
| } | ||
| } else { | ||
| s[i] = v.integer != 0 | ||
| } |
There was a problem hiding this comment.
| if len(v.string) != 0 { | |
| s[i], err = strconv.ParseBool(v.string) | |
| if err != nil { | |
| s[i] = false | |
| } | |
| } else { | |
| s[i] = v.integer != 0 | |
| } | |
| s[i], _ = v.AsBool() |
We can simply re-use .AsBool() here and ignore its error.
message_test.go
Outdated
| values := []RedisMessage{{string: "true", typ: '+'}} | ||
| if ret, _ := (RedisResult{val: RedisMessage{typ: '*', values: values}}).AsBoolSlice(); !reflect.DeepEqual(ret, []bool{true}) { | ||
| t.Fatal("AsBoolSlice not get value as expected") | ||
| } | ||
| values = []RedisMessage{{string: "false", typ: '+'}} | ||
| if ret, _ := (RedisResult{val: RedisMessage{typ: '*', values: values}}).AsBoolSlice(); !reflect.DeepEqual(ret, []bool{false}) { | ||
| t.Fatal("AsBoolSlice not get value as expected") | ||
| } |
There was a problem hiding this comment.
true/false strings are good use cases. Could you make AsBool() support them as well?
message_test.go
Outdated
| defer func() { | ||
| if r := recover(); r == nil { | ||
| t.Fatal("AsBoolSlice did not panic as expected") | ||
| } else if !strings.Contains(r.(string), "redis message type t is not an array") { | ||
| t.Fatal("AsBoolSlice did not panic with the expected message") | ||
| } | ||
| }() | ||
| (&RedisMessage{typ: 't'}).AsBoolSlice() |
There was a problem hiding this comment.
| defer func() { | |
| if r := recover(); r == nil { | |
| t.Fatal("AsBoolSlice did not panic as expected") | |
| } else if !strings.Contains(r.(string), "redis message type t is not an array") { | |
| t.Fatal("AsBoolSlice did not panic with the expected message") | |
| } | |
| }() | |
| (&RedisMessage{typ: 't'}).AsBoolSlice() |
duplicated.
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #500 +/- ##
=======================================
Coverage 95.58% 95.58%
=======================================
Files 80 80
Lines 33090 33107 +17
=======================================
+ Hits 31628 31645 +17
Misses 1261 1261
Partials 201 201 ☔ View full report in Codecov by Sentry. |
message.go
Outdated
| } | ||
| s := make([]bool, len(values)) | ||
| for i, v := range values { | ||
| s[i] = v.string == "1" |
There was a problem hiding this comment.
I think your original use of strconv.ParseBool was good. Why did you remove it completely? I believe we better add it to AsBool() and re-use AsBool() here.
|
This time I used AsBool function within AsBoolSlice and I think this approach is fine and better than strconv.ParseBool as the logic for parsing a boolean value is centralized and consistent across the codebase. Please let me now if there is any other feedback |
Hi. I added .AsBoolSlice() as requested