-
Notifications
You must be signed in to change notification settings - Fork 56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Make all array kinds nillable #2534
Changes from 1 commit
8a71b16
2690568
d1737e0
8a83889
d9953a9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1622,3 +1622,28 @@ func TestNormalValue_ToArrayOfNormalValues(t *testing.T) { | |
}) | ||
} | ||
} | ||
|
||
// This test documents a bug where array values | ||
// were not returning the correct value for IsNillable | ||
// and were also not convertible to a normal nil kind. | ||
func TestArrayValue_IsNillable(t *testing.T) { | ||
fieldKinds := []FieldKind{ | ||
FieldKind_BOOL_ARRAY, | ||
FieldKind_INT_ARRAY, | ||
FieldKind_FLOAT_ARRAY, | ||
FieldKind_STRING_ARRAY, | ||
FieldKind_NILLABLE_BOOL_ARRAY, | ||
FieldKind_NILLABLE_INT_ARRAY, | ||
FieldKind_NILLABLE_FLOAT_ARRAY, | ||
FieldKind_NILLABLE_STRING_ARRAY, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. todo: sorry, but I'm trying to figure out what this PR actually tries to achieve.
At the moment So please clarify which of those 4 types you are changing and how. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a discord conversation around this, all the scalar arrays are nillable atm, the enum values are just stuck with the old names. e.g. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At the moment, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. looks like I missed this discord conversation. All right then |
||
} | ||
|
||
for _, kind := range fieldKinds { | ||
assert.True(t, kind.IsNillable()) | ||
|
||
v, err := NewNormalNil(kind) | ||
require.NoError(t, err) | ||
|
||
assert.True(t, v.IsNil()) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: I'm not sure that this is correct. Please wait before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good here. I got mixed up because of us not updating the field kinds.