-
Notifications
You must be signed in to change notification settings - Fork 100
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
Register Go int as long in type resolver #423
Conversation
@@ -550,3 +550,94 @@ func TestEncoder_UnionInterfaceNotInSchema(t *testing.T) { | |||
|
|||
assert.Error(t, err) | |||
} | |||
|
|||
func TestEncoder_UnionResolver(t *testing.T) { |
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 didn't find any tests testing this behavior so I added it, specifically the test case Go int as Avro long
tests the line added in this PR. It's written in a different style than the other tests in this repo, if you prefer I can split it up into separate tests.
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.
Sorry for the delay. Internets went away for a while.
@@ -26,6 +26,7 @@ func NewTypeResolver() *TypeResolver { | |||
r.Register(string(Int), int16(0)) | |||
r.Register(string(Int), int32(0)) | |||
r.Register(string(Int), int(0)) | |||
r.Register(string(Long), int(0)) |
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.
Should this only be for 64 but systems?
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 don't think so, let me explain.
In case of encoding, you can encode int32
and int64
as long
, both should be fine. And this is enabled now because TypeResolver.Name(int)
will return long
as a possible type for encoding int
.
Now, in the case of decoding it's only safe to decode long
into int64
. This is till the case though, because in the next line we register int64
as long
, so TypeResolver.Type("long")
will still return int64
, as it was before.
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.
Fair point
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.
LGTM 👏🏻 Thanks for sorting this.
I noticed that
int
types are not resolved aslong
in unions, so I registered it in the type resolver.Follow-up to #422.
Related to #421.