-
Notifications
You must be signed in to change notification settings - Fork 2
Add plain enum support #8
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
Conversation
- Add src/enum.zig with pack/unpack functions for all enum types - Support plain enums: enum { foo, bar } - Support explicit backing type enums: enum(u8) { foo = 1, bar = 2 } - Support mixed enums with auto and explicit values - Support optional enums with null handling - Update any.zig to handle .@"enum" type info - Add enum methods to Packer/Unpacker APIs - Add comprehensive tests for all enum variations Enums are serialized as their underlying integer values using @intFromEnum/@enumFromInt conversions. Backing type is automatically determined via @typeinfo(T).@"enum".tag_type.
WalkthroughAdds a new enum serialization module and integrates enum encoding/decoding into the generic any-type pack/unpack system and the MsgPack API, re-exporting enum helpers and adding Packer/Unpacker enum methods plus tests for plain, typed, and optional enums. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Packer
participant EnumModule as packEnum
participant Writer
Note over User,Packer: Encode enum (including optional/null)
User->>Packer: writeEnum(value)
Packer->>EnumModule: packEnum(Writer, T, value)
EnumModule->>Writer: maybePackNull() / packInt(tag)
Writer-->>EnumModule: ok
EnumModule-->>Packer: ok
Packer-->>User: ok
sequenceDiagram
autonumber
participant User
participant Unpacker
participant EnumModule as unpackEnum
participant Reader
Note over User,Unpacker: Decode enum (including optional/null)
User->>Unpacker: readEnum(T)
Unpacker->>EnumModule: unpackEnum(Reader, T)
EnumModule->>Reader: unpackInt() or detect null-marker
Reader-->>EnumModule: tag / null
EnumModule->>Unpacker: @enumFromInt(tag) or null
Unpacker-->>User: value:T
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Claude finished @lalinsky's task —— View job Code Review AnalysisReviewing the enum support implementation for msgpack.zig: Todo List:
Code Review SummaryThis PR adds comprehensive enum support to msgpack.zig with a well-structured implementation. Here's my detailed analysis: ✅ Strengths1. Code Quality & Architecture
2. Comprehensive Type Support
3. Test Coverage
🟡 Minor Observations1. Performance Considerations
2. Error Handling
3. API Design
🔒 Security Assessment
📝 Recommendations1. Documentation Enhancement (Optional) 2. Error Handling Clarification (Minor) 🎯 Overall AssessmentThis is a high-quality implementation that:
The implementation demonstrates excellent understanding of both Zig's type system and msgpack serialization requirements. The code is ready for merge. |
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.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/any.zig
(4 hunks)src/enum.zig
(1 hunks)src/msgpack.zig
(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: claude-review
🔇 Additional comments (9)
src/enum.zig (1)
112-130
: LGTM! Well-structured edge case testGood test coverage for mixed enums with both explicit and auto-assigned values. The test properly validates that auto-assigned values work correctly after explicit ones.
src/any.zig (4)
37-40
: LGTM! Clean integration of enum supportThe imports are properly organized and follow the existing pattern for other type handlers.
63-63
: LGTM! Correct enum size calculationThe enum case is properly integrated into the size calculation switch statement.
113-113
: LGTM! Proper enum packing integrationThe enum case correctly delegates to the specialized
packEnum
function.
134-134
: LGTM! Proper enum unpacking integrationThe enum case correctly delegates to the specialized
unpackEnum
function.src/msgpack.zig (4)
68-72
: LGTM! Clean enum module exportsThe enum-related functions are properly exported following the existing pattern for other type handlers.
152-155
: LGTM! Consistent Packer method implementationThe
writeEnum
method follows the established pattern of other write methods in the Packer type.
244-247
: LGTM! Consistent Unpacker method implementationThe
readEnum
method follows the established pattern of other read methods in the Unpacker type.
321-376
: LGTM! Comprehensive enum test coverageExcellent test suite covering typed enums, plain enums, and optional enums (both null and non-null cases). The tests properly validate round-trip encoding and decoding.
@claude address coderabbit's review comments |
Claude finished @lalinsky's task —— View job Addressing CodeRabbit Review Comments ✅I've successfully fixed both issues identified by CodeRabbit in the enum support implementation. Todo List:
Fixes AppliedFix 1:
Fix 2:
Additional ImprovementsNew Comprehensive Tests (lines 186-228)
Changes Summary
The fixes address both CodeRabbit review comments exactly as suggested, ensuring proper optional enum support while maintaining backward compatibility with existing non-optional enum functionality. --- | View commit |
- Fix getEnumSize to properly handle optional enum types - Fix unpackEnum to support null handling for optional enums - Add comprehensive tests for optional enum functionality - Addresses CodeRabbit review feedback Co-authored-by: Lukáš Lalinský <lalinsky@users.noreply.github.com>
@claude see how we handle optionals elsewhere, there is a framework for this, you don't need the custom reader, you can read optional int. |
Claude finished @lalinsky's task —— View job Refactoring Optional Enum Handling ✅Todo List:
SummarySuccessfully refactored the enum optional handling to use the existing msgpack.zig framework patterns instead of custom reader logic: Key Improvements:
Framework Alignment:
The refactored code is cleaner, more maintainable, and correctly integrated with the existing msgpack.zig optional handling framework. Branch: feat/plain-enum-support | Commit: 331dac8 |
- Remove custom backup reader mechanism (44 lines removed) - Use maybeUnpackNull(header, T) pattern like int.zig - Simplify getEnumSize to handle optionals recursively - Follow established unpackInt dispatch pattern for header handling - Maintains full compatibility with existing tests Co-authored-by: Lukáš Lalinský <lalinsky@users.noreply.github.com>
@claude you can use unpackInt directly, just need to construct the optional tag type |
Claude finished @lalinsky's task —— View job Optimizing enum optional handling using unpackInt directly ✅ Todo List:
SummarySuccessfully optimized the enum unpacking implementation as suggested! 🎯 Key Improvements:
The optimization: // Before: Complex 31-line function with manual header parsing
// After: Simple 15-line function leveraging unpackInt
pub fn unpackEnum(reader: anytype, comptime T: type) !T {
const Type = assertEnumType(T);
const tag_type = @typeInfo(Type).@"enum".tag_type;
// Construct the optional tag type to match T's optionality
const OptionalTagType = if (@typeInfo(T) == .optional) ?tag_type else tag_type;
// Use unpackInt directly with the constructed optional tag type
const int_value = try unpackInt(reader, OptionalTagType);
// Convert to enum, handling optional case
if (@typeInfo(T) == .optional) {
return if (int_value) |value| @enumFromInt(value) else null;
} else {
return @enumFromInt(int_value);
}
} This approach is much more maintainable and follows the established patterns in the msgpack.zig codebase. The CI will validate that all existing tests continue to pass. Branch: feat/plain-enum-support | Commit: c5f98d2 |
- Construct optional tag type (?tag_type vs tag_type) based on enum optionality - Delegate all msgpack parsing logic to existing unpackInt function - Simplify unpackEnum from 31 lines to 15 lines - Remove complex header parsing and format-specific handling - Clean up unused imports (hdrs, NonOptional, maybeUnpackNull) Co-authored-by: Lukáš Lalinský <lalinsky@users.noreply.github.com>
Summary
src/enum.zig
module with pack/unpack functionssrc/any.zig
andsrc/msgpack.zig
to integrate enum supportEnum Types Supported
enum { foo, bar }
(uses auto-determined backing type like u1)enum(u8) { foo = 1, bar = 2 }
enum(u8) { first = 10, second, third = 20 }
(auto + explicit values)?MyEnum
with null handlingImplementation Details
@intFromEnum()
and@enumFromInt()
for conversions@typeInfo(T).@"enum".tag_type
API Added
Test plan
src/enum.zig
src/msgpack.zig
All 129 tests pass including the new enum functionality.
Summary by CodeRabbit
New Features
Tests