-
Notifications
You must be signed in to change notification settings - Fork 11
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
support for binary marshaler/unmarshaler #17
Conversation
Thanks! Will take a look in the coming days |
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.
time.Duration & time.Time already implements Text/BinaryMarshaler - should we use it? it would reduce the amount of code and make things more compatible with the go std. right now, i am not. it is more standard, and protected by go-1 guarantees. however, it will break existing applications since the string will be in a different format.
Only time.Time
does. Let's do it 🙂 (and see if someone complains)
technically redis is a "binary" protocol (can send arbitrary bytes over stream). should i use BinaryMarshaler instead or both? the only reason why i am using textmarshaler is bc the redis protocol itself is utf8, but this is all semantic - it's up to you.
It looks like go-redis
uses only the Binary[Un]Marshaler
interfaces, see my comment. I'm happy to go with what you find best... If anyone has trouble, we'll see complaints and think about finding common grounds 😁
gtrsconvert/convert.go
Outdated
case encoding.TextMarshaler: | ||
txt, err := v.MarshalText() | ||
if err != nil { | ||
return nil, SerializeError{ | ||
Field: fieldType.Name, | ||
Value: fieldValue.Interface(), | ||
Err: err, | ||
} | ||
} | ||
out[fieldName] = string(txt) |
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.
Based on what I found in the go-redis
library it uses only Binary[Un]Marshaller
interfaces. On one side we'd allow text types to be send, on the other if a struct implements both Text/Binary, we'd prioritize text whereas the redis client would've used binary 🤔 Not sure... what do you think?
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.
didn't realize the credit client used binary - I think we should use the same.
utils.go
Outdated
type ConvertibleFrom = gtrsconvert.ConvertibleFrom | ||
|
||
type FieldParseError = gtrsconvert.FieldParseError | ||
type SerializeError = gtrsconvert.SerializeError |
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.
👍🏻 Because I wouldn't like exposing internal stuff otherwise
hmm, so i guess there's some weirdness with how the library currently is (encodes time as human readable, while binarymarshaler/unmarshaler doesn't) my thoughts here - since the redis library uses MarshalBinary, we should use MarshalBinary to keep consistency accross existing codebase which rely on MarshalBinary behavior for values users interested in having a human readable representation of "time" in their packages can implement their own type wrapper around time.Time which uses TextMarshaler, or use the provided "AsciiTime", however i've pushed with this change. |
this pr does a few things
addresses #16
note that the behavior is a bit funky - if either
type
OR*type
implement the interfaces, they will be used.this is so we can properly support embedded map and slice types with default values.