-
Notifications
You must be signed in to change notification settings - Fork 101
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
add support for recursive schemas & structs #413
Conversation
1032a2b
to
cd10c42
Compare
Any particular reason you chose this approach over passing a DecoderContext/EncoderContext through? |
Just a matter of personal way of coding, I tend to have the functions on a struct rather than having free-floating with large names |
Fair. This has however created some inconsistency in the module as a whole. For example |
Pushed improvements. I do have one question: What does |
Interfaces are split into 2 types, an empty interface (interface{} or any) |
The issue this poses is that it fundamentally changes the codebase far beyond what is required to implement the feature. In fact I see a couple ways to do it without putting everything under a struct. Part of this also subtly changes how this works in ways I don't fully understand the impact of yet. I am not making any argument of the relative benefits of either approach. Both are perfectly valid, and were I to completely rewrite today, perhaps a different approach would be chosen. As an example, this changes the benchmark of the library for the worse (~224ns to ~230ns), but it would be very difficult to pin down exactly were the degraded performance comes from, small as it may be. |
So you would not have decoderCreator or encoderCreator but replace the parameter |
Yes. That seems like it should work. |
1e705e5
to
939e15a
Compare
@nrwiersma pushed using |
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.
Nice addition, thanks. A couple comments.
Co-authored-by: Nicholas Wiersma <nick@wiersma.co.za>
Co-authored-by: Nicholas Wiersma <nick@wiersma.co.za>
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 the contribution.
No description provided.