Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR adds generic, reflection-aware ZSS serialization, a draft 2020-12 ZSS-to-JSON-Schema converter, a schema-generation CLI, and updates tests to the new generic API and metadata model. ChangesZSS JSON Schema Generation and Type-Driven Serialization
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Deploying zog with
|
| Latest commit: |
931bea4
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://acb3ab35.zog-3a0.pages.dev |
| Branch Preview URL: | https://feat-zss.zog-3a0.pages.dev |
8316554 to
04e0062
Compare
Greptile SummaryThis PR adds JSON Schema support for ZSS documents. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (4): Last reviewed commit: "chore: fix zog schema regex" | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
cmd/zssschema-gen/main_test.go (1)
15-93: ⚡ Quick winAdd coverage for the
-base-urlcontract.Line 44 introduces a user-facing flag, but current tests never assert
$idgeneration for non-default base URLs.Suggested test addition
+func TestRunUsesCustomBaseURL(t *testing.T) { + var stdout, stderr bytes.Buffer + + err := run([]string{ + "-version", "0.0.1", + "-inline", + "-base-url", "https://example.dev/custom/zss/", + }, &stdout, &stderr) + + require.NoError(t, err) + assert.Empty(t, stderr.String()) + + var schema map[string]any + require.NoError(t, json.Unmarshal(stdout.Bytes(), &schema)) + assert.Equal(t, "https://example.dev/custom/zss/0.0.1/schema.json", schema["$id"]) +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/zssschema-gen/main_test.go` around lines 15 - 93, Add a test that exercises the -base-url flag and asserts it affects the generated $id: call run(...) with "-version", a valid version, "-inline" (or "-out" for file path) and "-base-url", capture stdout/stderr and unmarshal JSON, then assert schema["$id"] equals the provided base URL plus "/zss/<version>/schema.json; e.g. add TestRunUsesBaseURL (or extend TestRunWritesSchemaToStdoutWithInline) to call run with "-base-url", verify stderr is empty, and assert the exact $id value and that stdout begins with the expected JSON prefix. Ensure you reference the existing run(...) helper and use the same json.Unmarshal checks as other tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkgs/zss/jsonschema/draft2020_12/jsonschema.go`:
- Around line 99-113: The UnknownKindConverter may legally return (nil, nil)
which leads to panics when later code writes to out or when c.applyProcessors is
called; update the branch that calls c.opts.UnknownKindConverter in method where
out is assigned so that after calling c.opts.UnknownKindConverter(schema) you
normalize a nil out to an empty Schema (e.g., if out == nil { out = Schema{} })
before any use or before calling c.applyProcessors(out, schema) and before the
function returns; reference the UnknownKindConverter call site, the local
variable out, the applyProcessors method, and the Schema type when making this
change.
- Around line 134-151: The required slice is populated by iterating the map
schema.Fields which yields nondeterministic order; before assigning it into
out["required"] (after building properties in the loop that calls
c.convertSchema and uses propertyName(schema.FieldMeta[name], name)), sort the
required slice (e.g., using sort.Strings) so the emitted JSON Schema has a
deterministic order; ensure you import the sort package if needed and perform
the sort right before the line that sets out["required"] = required.
In `@pkgs/zss/toZSS_processors_test.go`:
- Line 65: The tests call zog.EXPERIMENTAL_TO_ZSS with incorrect generic type
parameters (e.g., int for schemas built with zog.String()), so update each test
invocation of EXPERIMENTAL_TO_ZSS to use the schema root's actual Go type: for
schemas created via zog.String() use string, for zog.Int() use int, for
zog.Bool() use bool, etc.; locate usages of EXPERIMENTAL_TO_ZSS in
toZSS_processors_test.go (lines shown in the review) and replace the mismatched
type arguments so the generic T matches the schema root type for each test case.
In `@schemas/zss/0.0.1/schema.json`:
- Line 34: The schema's "pattern" values in schemas/zss/0.0.1/schema.json use
Python-style named groups `(?P<name>...)` which are not portable for
draft-2020-12; update the generator that emits these regexes so it replaces
`(?P<...>...)` with standard groups (either non-capturing `(?:...)` or plain
`(...)` as appropriate) for all occurrences (the pattern properties currently
containing `(?P<id>...)` and `(?P<version>...)`), then regenerate the schema so
the file contains only JSON-Schema-compatible regex syntax.
In `@toZSS.go`:
- Around line 107-119: In fieldTypeForShapeKey, guard against empty keys and
avoid the fixed 32-byte buffer that can be out-of-bounds: if key == "" return
(reflect.StructField{}, false); to uppercase the first byte when it's lowercase,
allocate a byte slice of length len(key) (or convert to []byte(key)), modify
index 0, and build the string from that slice (instead of using a fixed [32]byte
and slicing it). Keep the existing indirectType and t.Kind() checks and then
call t.FieldByName(fieldName).
---
Nitpick comments:
In `@cmd/zssschema-gen/main_test.go`:
- Around line 15-93: Add a test that exercises the -base-url flag and asserts it
affects the generated $id: call run(...) with "-version", a valid version,
"-inline" (or "-out" for file path) and "-base-url", capture stdout/stderr and
unmarshal JSON, then assert schema["$id"] equals the provided base URL plus
"/zss/<version>/schema.json; e.g. add TestRunUsesBaseURL (or extend
TestRunWritesSchemaToStdoutWithInline) to call run with "-base-url", verify
stderr is empty, and assert the exact $id value and that stdout begins with the
expected JSON prefix. Ensure you reference the existing run(...) helper and use
the same json.Unmarshal checks as other tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9b4c6cf6-bb4f-416b-8a5b-421a0fc94969
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (20)
cmd/zssschema-gen/main.gocmd/zssschema-gen/main_test.gogo.modpkgs/zss/core/zss_structures.gopkgs/zss/jsonschema/draft2020_12/jsonschema.gopkgs/zss/jsonschema/jsonschema.gopkgs/zss/jsonschema/jsonschema_test.gopkgs/zss/jsonschema/shared/schema.gopkgs/zss/schema/zss_document_schema.gopkgs/zss/toZSS_kinds_test.gopkgs/zss/toZSS_metadata_meta_test.gopkgs/zss/toZSS_metadata_nometa_test.gopkgs/zss/toZSS_processors_test.gopkgs/zss/toZSS_serialization_meta_test.gopkgs/zss/toZSS_struct_shape_test.gopkgs/zss/toZSS_test.gopkgs/zss/toZSS_type_metadata_test.gorecursive.goschemas/zss/0.0.1/schema.jsontoZSS.go
|
@greptile please review again |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Around line 18-19: The schema-gen Make target is invoking
cmd/zssschema-gen/main.go without the required version flag, so update the
schema-gen recipe to pass the schema version argument expected by the generator.
Use the schema generator entrypoint in cmd/zssschema-gen/main.go and ensure make
schema-gen supplies -version so it can regenerate
docs/static/zss/<version>/schema.json instead of failing immediately.
In `@toZSS.go`:
- Around line 116-122: The field lookup in toZSS should resolve struct tag
aliases before trying ASCII case guessing. Update the field-resolution logic
around t.FieldByName so it first checks tags such as json/zog against the
incoming key (for example full_name -> FullName), then falls back to the
existing first-letter capitalization behavior. Keep the fix localized to the
field lookup path used by FieldMeta so downstream schema conversion receives the
correct field metadata.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ade2812b-0274-4b5c-8987-cccfefe653b3
📒 Files selected for processing (8)
Makefilecmd/zssschema-gen/main.gocmd/zssschema-gen/main_test.godocs/static/zss/0.0.1/schema.jsonpkgs/zss/core/zss_structures.gopkgs/zss/jsonschema/draft2020_12/jsonschema.gopkgs/zss/jsonschema/jsonschema_test.gotoZSS.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkgs/zss/core/zss_structures.go
- cmd/zssschema-gen/main_test.go
- pkgs/zss/jsonschema/jsonschema_test.go
- cmd/zssschema-gen/main.go
- pkgs/zss/jsonschema/draft2020_12/jsonschema.go
Summary by CodeRabbit
New Features
make schema-genshortcut for schema generation.Bug Fixes
Tests
Chores