Conversation
WalkthroughAdds an experimental public interface for external custom ZOG schemas, a wrapper type delegating to implementations, and a constructor. Also adds a comprehensive test suite exercising the new wrapper, coercion, validation, and nested scenarios. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant CS as CustomSchema (wrapper)
participant Impl as EXPERIMENTAL_PUBLIC_ZOG_SCHEMA (impl)
participant Ctx as SchemaCtx
Caller->>CS: Use(impl) -> *CustomSchema
Caller->>CS: process(ctx)
CS->>Impl: Process(ctx)
Caller->>CS: validate(ctx)
CS->>Impl: Validate(ctx)
Caller->>CS: getType()
CS->>Impl: GetType()
Impl-->>CS: ZogType
CS-->>Caller: ZogType
Caller->>CS: setCoercer(func)
CS->>Impl: SetCoercer(func)
note over CS,Impl: CustomSchema simply delegates calls to the provided implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Points to review:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
custom.go (2)
94-101: Rename and document the exported interface; avoid leaking internals in its signature
- Go style: exported type names shouldn’t be SCREAM_CASE. Use a clear CamelCase name.
- Public API shouldn’t expose internals package types directly. Re-export the context type locally and use that in the interface.
Apply this diff:
-// Experimental API -type EXPERIMENTAL_PUBLIC_ZOG_SCHEMA interface { - Process(ctx *p.SchemaCtx) - Validate(ctx *p.SchemaCtx) - GetType() zconst.ZogType - SetCoercer(c CoercerFunc) -} +// Experimental: public interface for plugging in external ZOG schemas. +// Subject to change without notice. +type ExperimentalSchema interface { + Process(ctx *SchemaCtx) + Validate(ctx *SchemaCtx) + GetType() zconst.ZogType + SetCoercer(c CoercerFunc) +}Add this alias near the top of the file (outside the changed hunk) to decouple callers from internals:
// Re-export to keep the public API stable and avoid importing internals from user code. type SchemaCtx = p.SchemaCtx
107-110: Tighten struct type, add short doc, and assert interface conformance
- Switch field type to the renamed interface.
- Add a compile-time assertion to prevent regressions.
-// Experimental API -type CustomSchema struct { - schema EXPERIMENTAL_PUBLIC_ZOG_SCHEMA -} +// CustomSchema delegates to an underlying ExperimentalSchema. +type CustomSchema struct { + schema ExperimentalSchema +}Add this after the struct (outside the changed hunk if needed):
// Compile-time check that CustomSchema implements ExperimentalSchema. var _ ExperimentalSchema = (*CustomSchema)(nil)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
custom.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
custom.go (3)
internals/contexts.go (1)
SchemaCtx(134-144)zconst/consts.go (1)
ZogType(38-38)utils.go (1)
CoercerFunc(37-37)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
custom.go (1)
102-105: Critical: guard against nil to prevent runtime panics.The constructor doesn't validate that
schemais non-nil. Passingnilwill cause a panic when any method is invoked onc.schema. Additionally, the function name doesn't follow Go naming conventions (preferNewCustomSchema).Apply this diff to add nil guard and follow naming conventions:
-// Experimental API -func Use(schema EXPERIMENTAL_PUBLIC_ZOG_SCHEMA) *CustomSchema { +// NewCustomSchema wraps an ExperimentalSchema implementation. Panics if schema is nil. +func NewCustomSchema(schema ExperimentalSchema) *CustomSchema { + if schema == nil { + panic("NewCustomSchema: schema must not be nil") + } return &CustomSchema{schema: schema} }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
custom.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
custom.go (4)
internals/contexts.go (1)
SchemaCtx(134-144)zconst/consts.go (1)
ZogType(38-38)utils.go (1)
CoercerFunc(37-37)zogSchema.go (1)
ZogSchema(10-15)
🔇 Additional comments (3)
custom.go (3)
107-110: LGTM!The struct definition is appropriate with an unexported field to encapsulate the schema implementation.
112-112: LGTM!The compile-time interface check is a good practice to ensure
CustomSchemacorrectly implementsZogSchema.
114-128: LGTM!The delegation methods correctly implement the
ZogSchemainterface by forwarding calls to the wrapped schema's exported methods. This addresses the concern from the previous review about adding unexported adapter methods.
Oudwins
force-pushed
the
tristan/feat/custom-schemas
branch
from
6e41e14 to
2efe86e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
custom.go (2)
102-105: Guard against nil schema to prevent runtime panics.If
schemais nil, calling any method onCustomSchemawill panic. Adding a nil check provides a clearer failure point.// Experimental API func Use(schema EXPERIMENTAL_PUBLIC_ZOG_SCHEMA) *CustomSchema { + if schema == nil { + panic("Use: schema must not be nil") + } return &CustomSchema{schema: schema} }
94-100: Consider renaming to follow Go naming conventions.The interface name
EXPERIMENTAL_PUBLIC_ZOG_SCHEMAviolates Go conventions (prefer PascalCase likeExperimentalSchema). Since this is experimental, the internal type exposure may be acceptable, but the naming should follow idiomatic Go.
🧹 Nitpick comments (1)
custom_experimental_test.go (1)
42-48: Consider extracting duplicated validation logic.The pattern of creating an issue with message/code fallback is repeated three times. For test code this is acceptable, but extracting to a helper would reduce duplication.
func (s *testExperimentalSchema) addValidationIssue(ctx *p.SchemaCtx) { issue := ctx.Issue().SetMessage(s.errorMsg) if s.errorMsg == "" { issue.SetCode(zconst.IssueCodeCustom) } ctx.AddIssue(issue) }Also applies to: 62-68, 103-109
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
custom.go(1 hunks)custom_experimental_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
custom_experimental_test.go (8)
zconst/consts.go (4)
ZogType(20-20)IssueCodeCustom(41-41)TypeString(23-23)TypeNumber(24-24)internals/contexts.go (1)
SchemaCtx(134-144)struct.go (2)
Struct(42-46)Shape(35-35)custom.go (1)
Use(103-105)tutils/testIssueMessages.go (1)
FindByPath(44-52)internals/tests.go (1)
Required(114-121)slices.go (1)
Slice(44-53)pointers.go (1)
Ptr(31-35)
custom.go (4)
internals/contexts.go (1)
SchemaCtx(134-144)zconst/consts.go (1)
ZogType(20-20)utils.go (1)
CoercerFunc(21-21)zogSchema.go (1)
ZogSchema(10-15)
🔇 Additional comments (7)
custom.go (1)
107-128: LGTM!The
CustomSchemastruct and its delegation methods correctly implement theZogSchemainterface by forwarding calls to the wrapped schema. The compile-time interface assertion on line 112 is a good practice.custom_experimental_test.go (6)
116-128: Intentional but note the validator signature inconsistency.
Processpasses the dereferenced value (e.g.,string) to the validator, whileValidatepassesctx.ValPtr(e.g.,*string). This is handled in test validators that check for both types, but worth noting for maintainability.
143-180: Good test coverage for basic Use() and CustomSchema functionality.The tests properly verify both success and failure paths for Parse operations, including custom error messages.
264-298: Good test for zog tag mapping behavior.The test correctly verifies that when a struct field has a
zog:"id"tag, the input data should use the "id" key while the Shape uses the field name "ID".
432-445: Defensive error path checking is reasonable for experimental API.The test handles uncertainty about whether the error path will be "id" (from zog tag) or "ID" (from field name). This defensive approach is acceptable while the API stabilizes.
727-840: Excellent coverage of complex nested scenarios.These tests thoroughly exercise the integration of
CustomSchemawith nested structures, slices, and pointers, verifying correct data propagation through multiple levels of composition.
846-946: Comprehensive error handling test coverage.The tests properly verify error propagation for coercion failures, validation failures, and correct error path reporting in nested structures. The flexible assertions on lines 870-873 appropriately handle potential variations in error formatting.
1 participant
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.