-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
bug: correct handling of the zero int64 value #18729
Conversation
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.
Thanks for adding this! I left some suggestions for tests, and related non-panic bug, but I'm also happy to add those fixes in a separate PR if you prefer.
@@ -745,6 +745,8 @@ func (t FieldType) Zero() interface{} { | |||
return "" | |||
case TypeInt: | |||
return 0 | |||
case TypeInt64: |
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.
It would be great to add a test to help catch any future errors like this. I didn't want to overcomplicate by getting into reflection etc, but had a little play and came up with this:
func TestFieldTypeMethods(t *testing.T) {
unknownFormat := convertType(TypeInvalid).format
for i := TypeInvalid + 1; i < typeInvalidMax; i++ {
t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
if i.String() == TypeInvalid.String() {
t.Errorf("unknown type string for %d", i)
}
if convertType(i).format == unknownFormat {
t.Errorf("unknown schema for %d", i)
}
_ = i.Zero()
})
}
}
You'll notice, that required adding a new (unexported) type to the end of the enum:
// DO NOT USE. Any new values must be inserted before this value.
// Used to write tests that ensure type methods handle all possible values.
typeInvalidMax
That turned up that the String()
method is also missing a case for Int64
:
case TypeInt64:
return "int64"
@tomhjp looks good, thanks |
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.
I'll add the test in a separate PR. Thanks for the fix!
* bug: correct handling of the zero int64 value * Update changelog/18729.txt --------- Co-authored-by: valli_0x <[email protected]> Co-authored-by: Tom Proctor <[email protected]>
added TypeInt64 in Zero() method