Add json_rename support for enums.#2
Conversation
Added `TypeAnnotationProvider` and `TypeAnnotationOptions` for json metadata.
|
Just wanted to mention this shortly, I don't like how this is implemented and this is more-so a feature request than a pull request, because as of now runtime evaluation of the fields greatly worsens the performance. |
sakakibara
left a comment
There was a problem hiding this comment.
Thanks for following up with enum support. Keeping json_rename consistent across structs, tagged unions, and enums makes sense.
I reviewed the enum-specific commit on top of #1 and ran focused decode/encode probes. Please update this PR after the changes requested in #1 land so the final diff uses the revised annotation API. The current head also inherits #1's zig build test compile failure.
Before merging, please also:
- Add matching
encodeTypedsupport so enum annotations remain symmetric between decoding and encoding. - Use replacement semantics for renamed enum values, consistent with the existing
json_renamebehavior. A renamed value should not also accept its original Zig tag. - Apply the same precedence contract as #1: a non-null provider rename list overrides type-local renames, while null falls back to them.
- Add tests for type-local and provider-based renames, their precedence and fallback, streaming and tree decoding, typed encoding, and round trips.
- Validate invalid local and provider enum entries at comptime, including duplicate or ambiguous JSON names.
- Keep invalid-value diagnostics tied to the received JSON string rather than replacing it with an internal Zig tag name.
- Name the computed table
renames, name the resolved valuetag_name, and remove the comment that narrates how the lookup is performed. - Document enum renaming and add an
Unreleasedchangelog entry. - After #1 merges, rebase this branch onto the updated
mainbefore final review.
I left inline comments on the type-local lookup and replacement semantics.
| const renames = T.json_rename; | ||
| var entries: []const annotation_mod.TypeAnnotationRename = &.{}; | ||
|
|
||
| for (@typeInfo(@TypeOf(renames)).@"struct".decls) |d| { |
There was a problem hiding this comment.
This lookup does not see entries in a type-local annotation such as .{ .slow = "S" }. Those entries are anonymous-struct fields; struct.decls is empty.
Please build this table from struct.fields and run the same comptime annotation validation used by the other annotated types. A focused type-local enum test should cover both the mapping and invalid-entry validation.
|
|
||
| const entry = for (lookup_table) |rf| { | ||
| if (std.mem.eql(u8, rf.from, s)) break rf.to; | ||
| } else s; |
There was a problem hiding this comment.
Falling back to s here means that a renamed enum value still accepts its original Zig tag, so json_rename becomes an alias rather than a replacement.
Please keep this consistent with the existing rename behavior: use the renamed wire value when one is declared, and reject the original tag. Add a test that distinguishes those two inputs.
|
Sorry, I missed your note above that you view this as more of a feature request than a merge-ready pull request, and that you are not satisfied with the runtime cost of the current lookup. Please do not treat my review as an expectation that you rewrite the implementation. It can serve as acceptance criteria for the eventual feature. I would be happy to take this on after #1 is settled. If you would prefer to continue working on it yourself, please let me know; otherwise, no response is needed. The likely direction is to build the complete effective JSON-name-to-enum-value mapping at comptime and perform a single effective-name lookup at runtime. This can follow the same strategy as If I pick it up, I will benchmark annotated decoding against the unannotated path before settling the implementation. Thank you for raising the use case and providing the initial exploration. |
|
I could look further into this but I will have to finish #1 first and this is going to take some time to get into, but I would love to cooperate and work on this with you. I will share updates when there will be any. Thank you for your patience |
Self-explainatory, had to settle on some runtime checks for this, but other than that, this is a small change.
Depends on annotation provider, so please merge it first.