-
Notifications
You must be signed in to change notification settings - Fork 29
feat: experimental custom schemas #187
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
base: master
Are you sure you want to change the base?
Conversation
WalkthroughAdds an exported experimental interface for custom ZOG schemas, a wrapper type that delegates to it, and a constructor to use external implementations. No changes to existing core parsing/validation logic. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant CS as CustomSchema
participant Impl as EXPERIMENTAL_PUBLIC_ZOG_SCHEMA (impl)
participant Ctx as SchemaCtx
Client->>CS: Use(impl) -> *CustomSchema
Client->>CS: Process(Ctx)
CS->>Impl: Process(Ctx)
Client->>CS: Validate(Ctx)
CS->>Impl: Validate(Ctx)
Client->>CS: GetType()
CS->>Impl: GetType()
CS-->>Client: ZogType
Client->>CS: SetCoercer(func)
CS->>Impl: SetCoercer(func)
note over CS,Impl: CustomSchema delegates all calls to provided implementation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
✨ 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 |
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
🧹 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)
// Experimental API | ||
func Use(schema EXPERIMENTAL_PUBLIC_ZOG_SCHEMA) *CustomSchema { | ||
return &CustomSchema{schema: schema} | ||
} | ||
|
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.
🛠️ Refactor suggestion
Constructor: guard against nil and follow Go naming conventions
- Prevent nil interface panics at call sites.
- Prefer NewX naming for constructors.
-// Experimental API
-func Use(schema EXPERIMENTAL_PUBLIC_ZOG_SCHEMA) *CustomSchema {
- return &CustomSchema{schema: schema}
-}
+// Experimental: NewCustomSchema wraps a user-provided ExperimentalSchema.
+func NewCustomSchema(schema ExperimentalSchema) *CustomSchema {
+ if schema == nil {
+ panic("NewCustomSchema: schema must not be nil")
+ }
+ return &CustomSchema{schema: schema}
+}
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In custom.go around lines 102 to 106, change the constructor to follow Go naming
conventions and guard against a nil schema: rename Use to NewCustomSchema,
change its signature to return (*CustomSchema, error), validate that the passed
schema (EXPERIMENTAL_PUBLIC_ZOG_SCHEMA) is not nil and return a descriptive
error (e.g., fmt.Errorf("schema cannot be nil")) if it is, otherwise return
&CustomSchema{schema: schema}, error nil; update callers accordingly to handle
the error.
func (c *CustomSchema) Process(ctx *p.SchemaCtx) { | ||
c.schema.Process(ctx) | ||
} | ||
|
||
func (c *CustomSchema) Validate(ctx *p.SchemaCtx) { | ||
c.schema.Validate(ctx) | ||
} | ||
|
||
func (c *CustomSchema) GetType() zconst.ZogType { | ||
return c.schema.GetType() | ||
} | ||
|
||
func (c *CustomSchema) SetCoercer(coercer CoercerFunc) { | ||
c.schema.SetCoercer(coercer) | ||
} |
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.
💡 Verification agent
🧩 Analysis chain
Add unexported adapter methods to integrate with existing internal schema expectations (process/validate/getType/setCoercer)
If the engine expects the unexported method set (as used by Custom[T]) rather than the exported ones, this wrapper won’t plug into existing flows. Mirror the internal method names so CustomSchema can be used wherever a built-in schema is accepted.
Add these adapters (outside the changed hunk if necessary):
// Internal adapters to match existing engine expectations.
func (c *CustomSchema) process(ctx *p.SchemaCtx) { c.schema.Process(ctx) }
func (c *CustomSchema) validate(ctx *p.SchemaCtx) { c.schema.Validate(ctx) }
func (c *CustomSchema) getType() zconst.ZogType { return c.schema.GetType() }
func (c *CustomSchema) setCoercer(coercer CoercerFunc) { c.schema.SetCoercer(coercer) }
Run this to verify whether the codebase relies on the unexported method set or an internal interface that requires them:
🏁 Script executed:
#!/bin/bash
# Look for an internal interface or callsites using unexported methods
rg -nP -C3 '(?s)interface\s+\w+\s*{[^}]*\bprocess\s*\(\s*\*p\.SchemaCtx\)[^}]*\bvalidate\s*\(\s*\*p\.SchemaCtx\)[^}]*\bgetType\s*\(\)\s*zconst\.ZogType[^}]*\bsetCoercer\s*\('
rg -nP -C2 '\.\s*process\s*\(|\.\s*validate\s*\(|\.\s*getType\s*\(|\.\s*setCoercer\s*\('
Length of output: 14246
Add unexported adapter methods to CustomSchema
CustomSchema must implement the internal schema interface (lower-cased methods) used throughout the engine (e.g. c.process(...)
, c.getType()
, s.setCoercer(...)
). In custom.go, immediately after the existing exported methods, add:
// internal adapters for existing engine flows
func (c *CustomSchema) process(ctx *p.SchemaCtx) { c.schema.Process(ctx) }
func (c *CustomSchema) validate(ctx *p.SchemaCtx) { c.schema.Validate(ctx) }
func (c *CustomSchema) getType() zconst.ZogType { return c.schema.GetType() }
func (c *CustomSchema) setCoercer(coercer CoercerFunc) { c.schema.SetCoercer(coercer) }
This ensures CustomSchema satisfies all call sites and interfaces discovered in the codebase.
🤖 Prompt for AI Agents
In custom.go around lines 112 to 126, CustomSchema exposes exported methods but
is missing the lowercase internal adapter methods required by the engine; add
unexported adapter methods that simply forward to the exported ones so the type
satisfies internal interfaces: implement process(ctx *p.SchemaCtx) calling
c.schema.Process(ctx), validate(ctx *p.SchemaCtx) calling
c.schema.Validate(ctx), getType() zconst.ZogType returning c.schema.GetType(),
and setCoercer(coercer CoercerFunc) calling c.schema.SetCoercer(coercer).
Summary by CodeRabbit