Standalone enums to be declared and created #1280
Conversation
WalkthroughThe pull request adds support for standalone enums declared in the Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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: 1
🧹 Nitpick comments (1)
tests/Base.php (1)
347-382: Well-structured test with comprehensive coverage.The test method validates all critical aspects of x-sdk-enums support: presence, values, uniqueness, and coexistence with property-level enums.
Optional: Minor refactor for $enumNames usage
The
$enumNamesvariable at line 360 is only used once at line 381. You could simplify by removing it and usingcount($enums)directly, or by using it more throughout the test:- $enumNames = array_column($enums, 'name'); - // Find MockPlan enum from x-sdk-enums $mockPlanEnum = null; foreach ($enums as $enum) { if ($enum['name'] === 'MockPlan') { $mockPlanEnum = $enum; break; } } // x-sdk-enums are included $this->assertNotNull($mockPlanEnum, 'x-sdk-enums should be included in response enums'); $this->assertArrayHasKey('enum', $mockPlanEnum); $this->assertEquals(['free', 'pro', 'scale', 'enterprise'], $mockPlanEnum['enum']); // x-sdk-enums don't duplicate $mockPlanCount = count(array_filter($enums, fn($e) => $e['name'] === 'MockPlan')); $this->assertEquals(1, $mockPlanCount, 'x-sdk-enums should not duplicate'); // regular property-level enums still work - $this->assertGreaterThan(1, count($enumNames), 'Both x-sdk-enums and property-level enums should be included'); + $this->assertGreaterThan(1, count($enums), 'Both x-sdk-enums and property-level enums should be included');
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Spec/Swagger2.phptests/Base.phptests/resources/spec.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-25T12:47:10.913Z
Learnt from: ItzNotABug
Repo: appwrite/sdk-generator PR: 1273
File: src/Spec/Swagger2.php:167-179
Timestamp: 2025-12-25T12:47:10.913Z
Learning: In src/Spec/Swagger2.php, when parsing x-oneOf entries from OpenAPI specs, assume the $ref key is always present by design. Do not implement special handling for missing $ref as a normal edge case; if $ref is missing, treat it as a broader specification validation failure and surface a clear error indicating the OpenAPI spec validation issue. This guideline applies specifically to this file-based parsing logic and should not be generalized as a runtime fallback.
Applied to files:
src/Spec/Swagger2.php
🧬 Code graph analysis (2)
tests/Base.php (2)
src/Spec/Swagger2.php (2)
Swagger2(7-688)getResponseEnums(620-671)src/Spec/Spec.php (1)
getResponseEnums(202-202)
src/Spec/Swagger2.php (1)
src/Spec/Spec.php (1)
getAttribute(144-159)
⏰ 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). (15)
- GitHub Check: build (8.3, KotlinJava11)
- GitHub Check: build (8.3, KotlinJava17)
- GitHub Check: build (8.3, Node16)
- GitHub Check: build (8.3, Go112)
- GitHub Check: build (8.3, Python310)
- GitHub Check: build (8.3, PHP80)
- GitHub Check: build (8.3, FlutterBeta)
- GitHub Check: build (8.3, FlutterStable)
- GitHub Check: build (8.3, Android14Java17)
- GitHub Check: build (8.3, DartBeta)
- GitHub Check: build (8.3, Android5Java17)
- GitHub Check: swift (server)
- GitHub Check: kotlin (server)
- GitHub Check: android (client)
- GitHub Check: apple (client)
🔇 Additional comments (1)
tests/resources/spec.json (1)
2261-2271: LGTM!The
x-sdk-enumstest fixture is correctly structured with the MockPlan enum. The values are appropriate for testing, and the missingkeysfield is acceptable since the code defaults it to an empty array.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/Base.php (2)
347-378: Test logic correctly validates x-sdk-enums integration.The test properly verifies that standalone enums from
x-sdk-enumsare parsed, merged without duplication, and contain the expected values.Optional: More idiomatic search pattern
Consider using
array_filterfor a more functional approach:- // Find MockPlan enum from x-sdk-enums - $mockPlanEnum = null; - foreach ($enums as $enum) { - if ($enum['name'] === 'MockPlan') { - $mockPlanEnum = $enum; - break; - } - } + // Find MockPlan enum from x-sdk-enums + $mockPlanEnums = array_filter($enums, fn($e) => $e['name'] === 'MockPlan'); + $mockPlanEnum = !empty($mockPlanEnums) ? reset($mockPlanEnums) : null;This approach is more concise and aligns with the pattern already used at line 377.
380-381: Consider strengthening the property-level enum assertion.The current check
assertGreaterThan(1, count($enumNames))only verifies multiple enums exist, but doesn't explicitly confirm that property-level enums (from paths/responses) are still being parsed. If the fixture contained multiplex-sdk-enumsentries but property-level enum parsing broke, this assertion would still pass.Strengthen by checking for a known property-level enum
- // regular property-level enums still work - $this->assertGreaterThan(1, count($enumNames), 'Both x-sdk-enums and property-level enums should be included'); + // regular property-level enums still work + $this->assertGreaterThan(1, count($enumNames), 'Both x-sdk-enums and property-level enums should be included'); + + // Explicitly verify a known property-level enum exists (e.g., from path parameters or response schemas) + // Replace 'ExpectedPropertyEnum' with an actual enum name from your spec.json fixture + $hasPropertyEnum = in_array('ExpectedPropertyEnum', $enumNames); + $this->assertTrue($hasPropertyEnum, 'Property-level enums should still be parsed alongside x-sdk-enums');Replace
'ExpectedPropertyEnum'with an enum that you know exists in the fixture's path/response definitions to robustly verify backward compatibility.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Spec/Swagger2.phptests/Base.php
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Spec/Swagger2.php
⏰ 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). (19)
- GitHub Check: build (8.3, Swift56)
- GitHub Check: build (8.3, Python39)
- GitHub Check: build (8.3, Python311)
- GitHub Check: build (8.3, PHP83)
- GitHub Check: build (8.3, Go118)
- GitHub Check: build (8.3, Node18)
- GitHub Check: build (8.3, Go112)
- GitHub Check: build (8.3, Node20)
- GitHub Check: build (8.3, FlutterStable)
- GitHub Check: build (8.3, CLINode20)
- GitHub Check: build (8.3, CLINode18)
- GitHub Check: build (8.3, KotlinJava8)
- GitHub Check: build (8.3, Android14Java17)
- GitHub Check: build (8.3, FlutterBeta)
- GitHub Check: build (8.3, Android5Java17)
- GitHub Check: build (8.3, DartStable)
- GitHub Check: android (client)
- GitHub Check: swift (server)
- GitHub Check: apple (client)
|
Not required atm. Will trace back sometime later. Moving to |
What does this PR do?
(Provide a description of what this PR does.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work.)
Related PRs and Issues
(If this PR is related to any other PR or resolves any issue or related to any issue link all related PR and issues here.)
Have you read the Contributing Guidelines on issues?
(Write your answer here.)
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.