Fix enum rendering in templates and remove enumName fallback hacks #1257
Conversation
This commit fixes how enums are rendered across all SDK templates by ensuring that parameter.enumName is always set at the source level, eliminating the need for fallback checks throughout the templates. Changes: - Fixed src/Spec/Swagger2.php to always set enumName with fallback to parameter name - Removed enumName fallback pattern from 15 template files across all SDKs - Simplified enum import logic in service templates - Updated example templates to use enumName directly Previously, enums were rendering incorrectly (e.g., ".light" instead of "Theme.light") because enumName could be empty when x-enum-name wasn't set in the OpenAPI spec. Now all SDKs correctly generate: - Dart/Flutter: Theme.light, Timezone.africaAbidjan, Output.jpg - Python: theme.LIGHT, timezone.AFRICA_ABIDJAN, output.JPG - PHP: Theme::LIGHT(), Timezone::AFRICAABIDJAN(), Output::JPG() - Kotlin/Android: Proper enum class references Affected templates: - Dart, Flutter, Python, PHP, Kotlin, Android, Web, React Native, Node, Deno
|
Warning Rate limit exceeded@ChiragAgg5k has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 55 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (10)
WalkthroughThis PR refactors enum handling across the codebase by introducing a consistent use of Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Areas requiring extra attention:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
templates/python/docs/example.md.twig (1)
6-13: Fix incorrect condition preventing enum imports in Python exampleLine 8 uses
{% if method == parameter.required %}, which compares an object to a boolean and will never be true. This blocks the entire enum import block from executing, causing generated Python examples to omit required enum imports.Change to:
{% if parameter.required %}This same bug appears in multiple templates (ruby, kotlin, android) and should be fixed everywhere it occurs.
templates/kotlin/docs/java/example.md.twig (1)
7-14: The enum import guard condition is indeed wrong and prevents imports from being emittedYour review correctly identifies a critical logic error: the condition
{% if method == parameter.required %}at line 9 compares the method object to a boolean, which is always false and prevents this block from executing.The same problematic condition exists in:
- templates/kotlin/docs/kotlin/example.md.twig:9
- templates/python/docs/example.md.twig:8
- templates/ruby/docs/example.md.twig:7
- templates/android/docs/kotlin/example.md.twig:9
- templates/android/docs/java/example.md.twig:9
The enumName usage itself is correct (line 12 imports via
parameter.enumName), but the wrapping guard prevents it from ever running. Change to{% if parameter.required %}to fix.
🧹 Nitpick comments (2)
templates/kotlin/src/main/kotlin/io/appwrite/models/Model.kt.twig (1)
5-8: Enum imports now correctly rely onproperty.enumNameUsing
property.enumName | caseUcfirstfor the Kotlin enum import matches howenumNameis populated inSwagger2::getDefinitions, so templates can rely on a single source of truth. You might optionally dedupe repeated imports across properties later, but the current change is sound.src/Spec/Swagger2.php (1)
209-235: Centralizing enumName fallback inparseMethodis correct and matches existing enum derivationSetting:
$param['enumName'] = $parameter['x-enum-name'] ?? $param['name']; ... $temp['enumName'] = $value['x-enum-name'] ?? $temp['name'];ensures all enum-bearing request parameters and body properties have a non-empty
enumNamethat templates can safely consume. This matches the previous effective behaviour ingetRequestEnums(enumNameor fallback toname), but moves the logic to parsing time so templates no longer need per-file fallbacks.
getRequestEnums()still does$enumName = $parameter['enumName'] ?? $parameter['name'];, which is now redundant but not incorrect; you could simplify that later if desired.Also applies to: 253-279
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/Spec/Swagger2.php(2 hunks)templates/android/docs/java/example.md.twig(2 hunks)templates/android/docs/kotlin/example.md.twig(2 hunks)templates/dart/docs/example.md.twig(1 hunks)templates/deno/src/services/service.ts.twig(1 hunks)templates/flutter/docs/example.md.twig(3 hunks)templates/kotlin/docs/java/example.md.twig(1 hunks)templates/kotlin/docs/kotlin/example.md.twig(1 hunks)templates/kotlin/src/main/kotlin/io/appwrite/models/Model.kt.twig(1 hunks)templates/node/src/services/template.ts.twig(1 hunks)templates/php/docs/example.md.twig(2 hunks)templates/php/src/Services/Service.php.twig(1 hunks)templates/python/docs/example.md.twig(1 hunks)templates/python/package/services/service.py.twig(1 hunks)templates/react-native/src/services/template.ts.twig(1 hunks)templates/web/src/services/template.ts.twig(1 hunks)
⏰ 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, Python310)
- GitHub Check: build (8.3, Python313)
- GitHub Check: build (8.3, AppleSwift56)
- GitHub Check: build (8.3, Ruby30)
- GitHub Check: build (8.3, WebChromium)
- GitHub Check: build (8.3, Python311)
- GitHub Check: build (8.3, Ruby31)
- GitHub Check: build (8.3, PHP80)
- GitHub Check: build (8.3, PHP83)
- GitHub Check: build (8.3, Node16)
- GitHub Check: build (8.3, KotlinJava8)
- GitHub Check: build (8.3, FlutterBeta)
- GitHub Check: build (8.3, FlutterStable)
- GitHub Check: build (8.3, DartStable)
- GitHub Check: build (8.3, Android5Java17)
- GitHub Check: build (8.3, Android14Java17)
- GitHub Check: swift (server)
- GitHub Check: apple (client)
- GitHub Check: android (client)
🔇 Additional comments (14)
templates/php/src/Services/Service.php.twig (1)
9-16: PHP enum imports correctly centralized onparameter.enumNameUsing
parameter.enumNamefor both theaddedcheck and the imported class (Enums\{{ parameter.enumName | caseUcfirst }}) matches the new parser behaviour and keeps the enum-name logic in one place. This should preserve behaviour while simplifying the template.templates/node/src/services/template.ts.twig (1)
4-10: Node service enum imports now consistently useparameter.enumNameThe new block:
{% if parameter.enumName not in added %} import { {{ parameter.enumName | caseUcfirst }} } from '../enums/{{ parameter.enumName | caseKebab }}'; {% set added = added|merge([parameter.enumName]) %} {% endif %}is aligned with the parser’s enumName fallback and keeps enum import naming consistent across the Node SDK. Looks good.
templates/flutter/docs/example.md.twig (1)
24-28: Flutter examples now correctly reference enums viaparameter.enumNameRouting enum arguments through
{{ parameter.enumName | caseUcfirst | overrideIdentifier }}.{{ ... | caseEnumKey }}aligns the examples with the actual generated Flutter enums and fixes the bare.value-style output. This change is consistent with the new enumName handling in the spec parser.Also applies to: 34-38, 48-52
templates/react-native/src/services/template.ts.twig (1)
8-15: React Native service enum imports aligned with centralizedenumNamehandlingThe new enum import block that uses
parameter.enumNamefor deduplication, path (caseKebab), and symbol (caseUcfirst) matches the Node service template and the parser’s enumName semantics. This keeps enum handling consistent across TS targets.templates/python/package/services/service.py.twig (1)
13-15: Enum import simplification aligns with broader refactor.The removal of fallback logic is consistent with the centralized approach. Ensure the upstream Swagger2.php change guarantees
enumNameis never empty to prevent invalid imports likefrom ..enums. import.templates/web/src/services/template.ts.twig (1)
9-11: TypeScript enum import refactor looks good.The simplified logic correctly relies on
parameter.enumNamebeing consistently populated upstream. Confirm that all OpenAPI parsing paths in Swagger2.php set this field to avoid missing enum imports.templates/android/docs/kotlin/example.md.twig (2)
11-13: Android Kotlin enum imports simplified correctly.Direct
parameter.enumNameusage aligns with the broader refactor. Verify the Swagger2.php fallback ensures this field is always populated.
41-41: Enum reference updated consistently.The parameter assignment correctly uses
parameter.enumNamedirectly, matching the import changes.templates/deno/src/services/service.ts.twig (1)
39-41: Deno enum import handling simplified appropriately.The change aligns with the centralized enumName approach across all platforms. Ensure upstream parsing always populates
enumName.templates/php/docs/example.md.twig (2)
11-13: PHP enum import logic correctly simplified.The removal of fallback logic is appropriate given the centralized fix. Verify that
parameter.enumNameis always set by the parser.
37-37: Enum parameter rendering updated consistently.Direct use of
parameter.enumNamematches the import changes.templates/dart/docs/example.md.twig (1)
25-25: Dart enum reference simplified correctly.The direct use of
parameter.enumNameis consistent with the broader refactor. Verify upstream parsing ensures this field is never empty.templates/android/docs/java/example.md.twig (2)
11-13: Android Java enum imports refactored appropriately.The simplified logic correctly assumes
parameter.enumNameis consistently populated. Confirm the Swagger2.php changes are in place.
45-46: Enum parameter usage updated consistently.Direct
parameter.enumNamereference matches the import simplification.
Summary
This PR fixes how enums are rendered across all SDK templates by ensuring that
parameter.enumNameis always set at the source level, eliminating the need for fallback checks throughout the templates.Problem
Previously, enums were rendering incorrectly in generated SDK documentation and code examples. For instance:
.light,.africaAbidjan,.jpg(missing enum class prefix)Theme.light,Timezone.africaAbidjan,Output.jpg✅This occurred because
parameter.enumNamecould be empty/null whenx-enum-namewasn't explicitly set in the OpenAPI specification, causing templates to output incomplete enum references.Solution
Instead of having fallback logic scattered across 15+ template files with patterns like:
{% if parameter.enumName is not empty %} {% set name = parameter.enumName %} {% else %} {% set name = parameter.name %} {% endif %}This PR centralizes the fallback logic in the source (
src/Spec/Swagger2.php) where parameters are parsed:Changes Made
Core Fix
enumNamealways has a valueTemplate Cleanup (16 files)
Removed fallback hacks from all templates:
Documentation Examples:
Service/SDK Templates:
Verification
Generated examples now correctly render enums across all SDKs:
Theme.light,Timezone.africaAbidjan,Output.jpgTheme.light,Timezone.africaAbidjan,Output.jpgtheme.LIGHT,timezone.AFRICA_ABIDJAN,output.JPGTheme::LIGHT(),Timezone::AFRICAABIDJAN(),Output::JPG()Impact
Test Plan
Summary by CodeRabbit