Skip to content
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

Merged
merged 4 commits into from
Jun 25, 2024

Conversation

adrianiacobghiula
Copy link
Contributor

No description provided.

@nrwiersma
Copy link
Member

Any particular reason you chose this approach over passing a DecoderContext/EncoderContext through?

@adrianiacobghiula
Copy link
Contributor Author

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 createDecoderOfMap() vs encoderCreator.ofMap() because of input parameters, long-names & cohesion

@nrwiersma
Copy link
Member

Fair. This has however created some inconsistency in the module as a whole. For example createDefaultDecoder still exists with the old signature. (d decoderCreator) newEfaceDecoder is actually a constructor but has been attached.

@adrianiacobghiula
Copy link
Contributor Author

Pushed improvements. I do have one question: What does eface stands for ?

@nrwiersma
Copy link
Member

Interfaces are split into 2 types, an empty interface (interface{} or any) eface and a populated (has methods) interface iface.

@nrwiersma
Copy link
Member

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.

@adrianiacobghiula
Copy link
Contributor Author

So you would not have decoderCreator or encoderCreator but replace the parameter cfg *frozenConfig to another object like DecoderContext (that contains the cfg *frozenConfig & the map ?) to every function createDecoderOfMap, decoderOfType, createDecoderOfMarshaler ?

@nrwiersma
Copy link
Member

Yes. That seems like it should work.

@adrianiacobghiula
Copy link
Contributor Author

@nrwiersma pushed using DecoderContext & EncoderContext

Copy link
Member

@nrwiersma nrwiersma left a 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.

codec.go Outdated Show resolved Hide resolved
codec.go Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
codec.go Outdated Show resolved Hide resolved
codec.go Outdated Show resolved Hide resolved
adrianiacobghiula and others added 2 commits June 25, 2024 08:19
Co-authored-by: Nicholas Wiersma <nick@wiersma.co.za>
Co-authored-by: Nicholas Wiersma <nick@wiersma.co.za>
Copy link
Member

@nrwiersma nrwiersma left a 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.

@nrwiersma nrwiersma merged commit 66aad10 into hamba:main Jun 25, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants