Reduce code duplication - #60
Conversation
|
Warning Review limit reached
Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR refactors OpenAPI/Swagger code generation and parsing in Zig. generator.zig and lib.zig consolidate version-specific wrapper functions into generic helpers. A new ident_utils.zig centralizes identifier validation. A new parse_helpers.zig provides shared array parsing, and v3.1/v3.2 model files re-export v3.0 type definitions instead of duplicating them. ChangesGenerator and Library Dispatch Consolidation
Shared Identifier Utilities
Model De-duplication Across OpenAPI Versions
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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)
src/generators/unified/ident_utils.zig (2)
28-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the shared
appendIdentifierwriter too.
isBareIdentifieris consumed by two near-identicalappendIdentifierimplementations inapi_generator.zigandmodel_generator.zig(escaping/quoting logic is byte-for-byte the same). Since this module already centralizes identifier classification, it could also host a genericappendIdentifier(buffer: *std.ArrayList(u8), allocator: std.mem.Allocator, name: []const u8) !voidhelper, letting both generators call a one-line wrapper instead of duplicating the ~20-line escape loop.♻️ Proposed shared helper
pub fn isBareIdentifier(name: []const u8) bool { if (name.len == 0 or !isIdentStart(name[0]) or isReservedIdent(name)) return false; for (name[1..]) |c| { if (!isIdentContinue(c)) return false; } return true; } + +pub fn appendIdentifier(buffer: *std.ArrayList(u8), allocator: std.mem.Allocator, name: []const u8) !void { + if (isBareIdentifier(name)) { + try buffer.appendSlice(allocator, name); + return; + } + try buffer.appendSlice(allocator, "@\""); + for (name) |c| { + switch (c) { + '\\', '"' => { + try buffer.append(allocator, '\\'); + try buffer.append(allocator, c); + }, + '\n' => try buffer.appendSlice(allocator, "\\n"), + '\r' => try buffer.appendSlice(allocator, "\\r"), + '\t' => try buffer.appendSlice(allocator, "\\t"), + else => try buffer.append(allocator, c), + } + } + try buffer.appendSlice(allocator, "\""); +}🤖 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 `@src/generators/unified/ident_utils.zig` around lines 28 - 34, The identifier handling is duplicated in the two `appendIdentifier` implementations, so centralize that shared escaping/quoting logic alongside `isBareIdentifier` in `ident_utils.zig`. Add a generic `appendIdentifier(buffer, allocator, name)` helper there, then update the `api_generator.zig` and `model_generator.zig` wrappers to delegate to it instead of keeping their own near-identical loops. Keep the existing `isBareIdentifier` behavior unchanged and use the new shared helper as the single source of truth for identifier output formatting.
1-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo unit tests for the newly-shared identifier logic.
isIdentStart/isIdentContinue/isReservedIdent/isBareIdentifiernow back identifier sanitization across both generators; a compact test block here would guard against regressions in generated Zig code's syntactic validity (e.g. reserved words, leading digits, non-ASCII bytes).🤖 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 `@src/generators/unified/ident_utils.zig` around lines 1 - 34, Add a small test block for the shared identifier helpers in ident_utils.zig so the new logic is covered. Create focused unit tests for isIdentStart, isIdentContinue, isReservedIdent, and isBareIdentifier that verify reserved words, leading digits, underscores, and non-ASCII bytes behave as expected. Keep the tests close to the helper functions so future changes to identifier sanitization in the generators are caught early.
🤖 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 `@src/lib.zig`:
- Around line 258-261: `parseToUnified` still repeats the same converter
initialization and `convert` call pattern that `convertDocument` was added to
remove. Update `parseToUnified` to reuse `convertDocument` for each parser
branch instead of manually creating `PARSER.init(allocator)` and calling
`.convert(doc)` four times, referencing the existing `convertDocument` helper
and the parser-specific types used in `parseToUnified`.
In `@src/models/common/parse_helpers.zig`:
- Around line 4-11: The parseArray helper in parse_helpers.zig only frees the
ArrayList backing storage on failure, so partially parsed items are leaked if
T.parseFromJson errors mid-loop. Update parseArray to clean up each successfully
parsed element on error by iterating the accumulated items and calling their
deinit logic before deinitializing the list, using the item type T’s deinit
behavior if available. Keep the fix localized to parseArray so callers like the
OpenAPI parsers automatically get safe rollback for Server, Tag, and
SecurityRequirement parsing.
---
Nitpick comments:
In `@src/generators/unified/ident_utils.zig`:
- Around line 28-34: The identifier handling is duplicated in the two
`appendIdentifier` implementations, so centralize that shared escaping/quoting
logic alongside `isBareIdentifier` in `ident_utils.zig`. Add a generic
`appendIdentifier(buffer, allocator, name)` helper there, then update the
`api_generator.zig` and `model_generator.zig` wrappers to delegate to it instead
of keeping their own near-identical loops. Keep the existing `isBareIdentifier`
behavior unchanged and use the new shared helper as the single source of truth
for identifier output formatting.
- Around line 1-34: Add a small test block for the shared identifier helpers in
ident_utils.zig so the new logic is covered. Create focused unit tests for
isIdentStart, isIdentContinue, isReservedIdent, and isBareIdentifier that verify
reserved words, leading digits, underscores, and non-ASCII bytes behave as
expected. Keep the tests close to the helper functions so future changes to
identifier sanitization in the generators are caught early.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e86f1139-359b-4dd5-bdc4-26e3bd0bfe4c
📒 Files selected for processing (15)
src/generator.zigsrc/generators/unified/api_generator.zigsrc/generators/unified/ident_utils.zigsrc/generators/unified/model_generator.zigsrc/lib.zigsrc/models/common/parse_helpers.zigsrc/models/v3.0/openapi.zigsrc/models/v3.1/externaldocs.zigsrc/models/v3.1/openapi.zigsrc/models/v3.1/server.zigsrc/models/v3.1/tag.zigsrc/models/v3.2/externaldocs.zigsrc/models/v3.2/openapi.zigsrc/models/v3.2/server.zigsrc/models/v3.2/tag.zig
1 participant
Summary by CodeRabbit
Refactor
Bug Fixes