Repository navigation
Preserve nil values when flattening argument lists - #250
Conversation
|
coverage: 96.173% (+0.003%) from 96.17% — yuefdev:fix/flatten-nil-values-sol into huandu:master |
huandu
left a comment
huandu
left a comment
There was a problem hiding this comment.
One small test-style suggestion; the Flatten fix itself looks correct.
| {[]interface{}{1, nil, []interface{}{nil, 2}}, []interface{}{1, nil, nil, 2}}, | ||
| {[]interface{}{[]int{}, []int(nil)}, nil}, | ||
| } { | ||
| if got := Flatten(tc.input); !reflect.DeepEqual(got, tc.want) { |
There was a problem hiding this comment.
Small consistency suggestion: the existing tests in this package use github.com/huandu/go-assert (for example, TestFlatten in modifiers_test.go), while this new test uses reflect.DeepEqual here and again on line 22. Could we switch these assertions to go-assert and drop the reflect import? Functionally this is fine; it just keeps the test suite consistent.
There was a problem hiding this comment.
I've switched these to go-assert and dropped the reflect import in f85bd2e. The nil values and SQL arguments are still covered, using the same assertion style as TestFlatten. go test ./... passes.
Preserve nil scalar elements in
FlattenandList.List([]interface{}{1, nil, 2})currently produces two placeholders and drops the SQL NULL argument, while an untyped nil input also contradicts Flatten's scalar contract.Represent a nil leaf as one flattened element while continuing to omit empty slices. The regression fails before the fix; full tests, vet and changed-file gofmt pass.