Serde options refactor #506
Conversation
pkg/sr/serde.go
Outdated
| // Load tserde based on the supplied ID. | ||
| t = s.loadIDs()[int(idopt.ID())] |
There was a problem hiding this comment.
The last commit adds the ability to supply EncodingOpt to Decode, simply to have parity with Encode, but I'm not convinced if it makes sense. I included it to run it by you, but I can revert the change if needed.
This is how it works. If you supply options ID and Index to Decode it will find tserde based solely on the supplied options, it will not decode the header. In fact, it will assume there is no header and that it's dealing only with the raw encoded bytes. What I don't like about this is that Serde can't encode bytes without a header unless you specify your own SerdeHeader implementation which skips prepending the header. It would allow you to track the ID and index separately from the encoded payload, although I'm not sure such a use case exists, so it might needlessly complicate the API.
pkg/sr/serde.go
Outdated
| t, ok := s.loadTypes()[reflect.TypeOf(v)] | ||
| if !ok || (t.encode == nil && t.appendEncode == nil) { | ||
| return b, ErrNotRegistered | ||
| func (s *Serde) AppendEncode(b []byte, v any, opts ...EncodingOpt) ([]byte, error) { |
There was a problem hiding this comment.
What's the point of having options at the point of encoding?
Same thought on decoding.
I'd like to avoid options on these functions if possible -- would it be possible to have a block of Dyn(Append)?Encode functions?
There was a problem hiding this comment.
Hey @twmb! First of all, sorry, this work fell through the cracks. I'll try to find some time over the weekend and clean up the code. IIUC you're proposing to have DynEncode and DynAppendEncode that take EncodingOpt and leave the existing encode functions as they are, correct? I'll try it out.
There was a problem hiding this comment.
As promised, I added the changes. It simplified the code quite a bit - no more ID option, no more special treatment of any option, no more options passed to encode and decode. Instead we now have DynEncode and related functions, which expect parameters id and index to determine the correct tserde.
I'm looking forward to hear your feedback 😉
There was a problem hiding this comment.
Random question now that I'm seeing this PR:
Is there any reason to require registering the types at all and then using DynEncode, vs. having top-level, non Serde method functions,
func Encode(id int, index []int, v any, enc func(any) ([]byte, error)) ([]byte, error)
func AppendEncode(b []byte, id int, index []int, v any, enc (func([]byte, any) ([]byte, error)) ([]byte, error)?
I'm wondering: what use case do you have that allows you to know the id and index you want to encode, but not know the encode function?
There was a problem hiding this comment.
Also just as a heads up -- I have a big sr update coming in #548, which should also move the needle on package stabilization.
There was a problem hiding this comment.
Oh well, this was another long delay, sorry about that. You know, it's the standard apology, being busy. I'll try to be more responsive now and help you resolve this PR.
I merged the changes from master and resolved the conflicts introduced in the PR you mentioned.
what use case do you have that allows you to know the id and index you want to encode, but not know the encode function?
It's not that I don't know the encode function, I do know it actually. It's that Serde also appends the header and I don't want to duplicate that logic outside of Serde. Another reason to register the type is that the next time the type gets decoded in a different path of the code, we already have a Serde that knows how to decode it, no need to fetch the schema.
There was a problem hiding this comment.
No worries on the delay, I'm just as slow in the past few months due to games and traveling and holidays and work :)
I'm not sure I still understand but I'm sure I would if I see what you're trying to do -- do you have an example of how you intend to use the dyn functions that can't be served with the current API?
There was a problem hiding this comment.
Here's the line where we call Serde.Encode and provide an ID of the schema to use. (For completeness, here's Serde.Decode).
I re-read your suggestion a few comments above and I think that having top-level, non Serde functions for Encode and AppendEncode that let you specify the ID and the encode func could work. The calls wouldn't go through the Serde type, but I realized we don't reuse the same object in the decode and encode paths of the code, so no harm done. And the solution feels cleaner than having DynEncode.
I'll see if I can get some time in the next few weeks to work this into the PR.
| appendEncode := t.appendEncode | ||
| if appendEncode == nil { | ||
| // Fallback to t.encode. | ||
| appendEncode = func(b []byte, v any) ([]byte, error) { |
There was a problem hiding this comment.
I think this causes an extra alloc in the encode path.
|
|
||
| b, err := s.header().AppendEncode(b, int(t.id), t.index) |
There was a problem hiding this comment.
Looks like this was lost (and not added in one of the new top level functions)
There was a problem hiding this comment.
Oh I see, it's used in the new call to AppendEncode below.
There was a problem hiding this comment.
I might rather have a little bit of duplication, not sure yet.
There was a problem hiding this comment.
Yep leaning duplication; I rebased on master and squashed all of your commits and did a few touchups in #742.
* Renames Opt to ClientOpt * Renames SerdeOpt to EncodingOpt * Adds SerdeOpt, with the ability to set the header that is always used * Adds top-level Encode/AppendEncode functions, to be used when you are dynamically encoding types and their ids / indices This is #506 by @lovromazgon, rebased on master, squashed into one commit, with minor modifications
* Renames Opt to ClientOpt * Renames SerdeOpt to EncodingOpt * Adds SerdeOpt, with the ability to set the header that is always used * Removes Serde.SetDefaults, in favor of the new (and more standard) NewSerde function * Adds top-level Encode/AppendEncode functions, to be used when you are dynamically encoding types and their ids / indices This is #506 by @lovromazgon, rebased on master, squashed into one commit, with minor modifications
|
Merged in #742, ty for the work (and patience over time 😅 😬) I think the main thing I want to think about before stabilizing the sr package is whether the top level Encode and AppendEncode functions should take all their args as fields, vs. some form that allows for future growth. I'm currently leaning to keep the current form (though maybe swap the |
|
@lovromazgon I'm swapping SerdeHeader before id,index in #743 |
This is a follow-up to #467 (comment).
This PR includes the following changes:
OpttoClientOptSerdeOpttoEncodingOptSerdeOpt, which will ultimately be used to set theSerdeHeader(this can only be included once Serde Header #467 is merged)IDwhich lets you specify which schema ID to use when encoding (specifically useful when encoding dynamic data)