feat: sdk for md #1265
feat: sdk for md #1265
Conversation
WalkthroughThis pull request introduces a new Docs language implementation to the SDK architecture. The changes include a new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
example.php (1)
278-283: Redundant catch blocks and misleading success message.The
Exceptionclass extendsThrowable, so the first catch block will handle all exceptions, making the secondThrowablecatch unreachable for exceptions. Additionally, line 285 will print "Example SDKs generated successfully" even after an error is caught, which is misleading.Consider consolidating to a single catch block and preventing the success message after errors:
} -catch (Exception $exception) { - echo 'Error: ' . $exception->getMessage() . ' on ' . $exception->getFile() . ':' . $exception->getLine() . "\n"; -} -catch (Throwable $exception) { +catch (Throwable $exception) { echo 'Error: ' . $exception->getMessage() . ' on ' . $exception->getFile() . ':' . $exception->getLine() . "\n"; + exit(1); } echo "Example SDKs generated successfully\n";
🧹 Nitpick comments (4)
templates/docs/typescript/README.md.twig (1)
66-72: Minor inconsistency in method index formatting.The method index at lines 69-70 doesn't use clickable markdown links, unlike the services section at line 33 which uses proper link syntax. Consider making them consistent for better navigation.
{%~ for method in service.methods %} -- {{ service.name | caseLower }}/{{ method.name | caseKebab }}.md - `{{ service.name | caseCamel }}.{{ method.name | caseCamel }}()` +- [`{{ service.name | caseCamel }}.{{ method.name | caseCamel }}()`](./{{ service.name | caseLower }}/{{ method.name | caseKebab }}.md) {%~ endfor %}templates/docs/typescript/method.md.twig (2)
99-129: Consider extracting repetitive code example logic.The three branches (
location, non-webAuth, andwebAuth) contain nearly identical parameter rendering logic repeated three times. This makes the template harder to maintain and increases the risk of inconsistencies if changes are needed.Consider extracting the parameter rendering into a Twig macro or a partial template to reduce duplication and improve maintainability.
225-225: Clarify await usage for different method types.Line 225 states all methods should use
awaitor.then(), butlocationtype methods (line 99-108) don't useawaitin the examples. This could confuse developers reading the documentation.Consider conditionalizing this note:
-- The method returns a Promise, so it should be used with `await` or `.then()` +{%~ if method.type != 'location' %} +- The method returns a Promise, so it should be used with `await` or `.then()` +{%~ endif %}src/SDK/Language/Docs.php (1)
87-101: Missing default case in getTypeName switch statement.If
$parameter['type']is an unhandled value, the switch falls through to return$parameter['type']directly (line 101), which may not be the intended TypeScript type representation.Consider explicitly handling
self::TYPE_STRINGandself::TYPE_BOOLEANcases for clarity:case self::TYPE_OBJECT: return 'object'; + case self::TYPE_BOOLEAN: + return 'boolean'; + case self::TYPE_STRING: + return 'string'; } return $parameter['type'];
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
example.php(2 hunks)src/SDK/Language/Docs.php(1 hunks)templates/docs/typescript/README.md.twig(1 hunks)templates/docs/typescript/method.md.twig(1 hunks)
🧰 Additional context used
🪛 PHPMD (2.15.0)
src/SDK/Language/Docs.php
75-75: Avoid unused parameters such as '$spec'. (undefined)
(UnusedFormalParameter)
134-134: Avoid unused parameters such as '$lang'. (undefined)
(UnusedFormalParameter)
⏰ 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). (20)
- GitHub Check: build (8.3, Python39)
- GitHub Check: build (8.3, KotlinJava11)
- GitHub Check: build (8.3, Node20)
- GitHub Check: build (8.3, WebNode)
- GitHub Check: build (8.3, Swift56)
- GitHub Check: build (8.3, Python310)
- GitHub Check: build (8.3, AppleSwift56)
- GitHub Check: build (8.3, Ruby30)
- GitHub Check: build (8.3, Python311)
- GitHub Check: build (8.3, Node16)
- GitHub Check: build (8.3, PHP83)
- GitHub Check: build (8.3, FlutterStable)
- GitHub Check: build (8.3, CLINode16)
- GitHub Check: build (8.3, FlutterBeta)
- GitHub Check: build (8.3, Android14Java17)
- GitHub Check: build (8.3, Android5Java17)
- GitHub Check: apple (client)
- GitHub Check: swift (server)
- GitHub Check: flutter (client)
- GitHub Check: android (client)
🔇 Additional comments (2)
example.php (1)
270-276: LGTM!The Docs SDK generation follows the established pattern used by other languages in this file.
src/SDK/Language/Docs.php (1)
166-187: Helper methods are correctly inherited from parent Language class.The methods
extractPermissionParts(public) andtoCamelCase(protected) are defined in theLanguageparent class and are properly accessible to theDocsclass through inheritance. This is the correct design and requires no changes.
| * @param array $parameter | ||
| * @return string | ||
| */ | ||
| public function getTypeName(array $parameter, array $spec = []): string |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the Docs.php file
find . -name "Docs.php" -type f 2>/dev/null | head -20
# Also look for Language class
find . -name "Language.php" -type f 2>/dev/null | head -20
# List the SDK directory structure if it exists
if [ -d "src/SDK" ]; then
find src/SDK -type f -name "*.php" | head -30
fiRepository: appwrite/sdk-generator
Length of output: 716
🏁 Script executed:
#!/bin/bash
# Read the Docs.php file to check the specific lines mentioned in the review
echo "=== DOCS.PHP (lines 50-200) ==="
head -200 src/SDK/Language/Docs.php | tail -150
echo -e "\n=== LANGUAGE.PHP (to see parent class definition) ==="
# Look for method definitions in Language.php
grep -n "public function getTypeName\|public function getParamExample" src/SDK/Language.php
# Check if they're abstract methods
grep -n "abstract.*function\|interface" src/SDK/Language.php | head -20Repository: appwrite/sdk-generator
Length of output: 5968
Add default cases to match expressions to prevent UnhandledMatchError.
Both match expressions in getParamExample() (lines ~145-150 and ~152-163) lack default cases. If $type contains an unexpected value, an UnhandledMatchError is thrown. Add default => $example, or an appropriate fallback to handle unknown types. The unused parameters $spec and $lang are required by the parent abstract method signatures and should be kept.
🧰 Tools
🪛 PHPMD (2.15.0)
75-75: Avoid unused parameters such as '$spec'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
In src/SDK/Language/Docs.php around lines 75 (and specifically fix the match
expressions in getParamExample around lines ~145-150 and ~152-163), the two
match expressions lack default cases which can throw UnhandledMatchError for
unexpected $type values; update each match to include a fallback arm such as
default => $example, (or another appropriate fallback) to safely handle unknown
types while keeping the unused $spec and $lang parameters intact as required by
the parent signatures.
| return match ($type) { | ||
| self::TYPE_ARRAY => '[]', | ||
| self::TYPE_FILE => 'file', | ||
| self::TYPE_INTEGER, self::TYPE_NUMBER => '0', | ||
| self::TYPE_BOOLEAN => 'false', | ||
| self::TYPE_OBJECT => '{}', | ||
| self::TYPE_STRING => "''", | ||
| }; |
There was a problem hiding this comment.
Missing default case in match expression.
If $type is an unexpected value (e.g., a custom type not handled), this match expression will throw an UnhandledMatchError. Consider adding a default fallback.
return match ($type) {
self::TYPE_ARRAY => '[]',
self::TYPE_FILE => 'file',
self::TYPE_INTEGER, self::TYPE_NUMBER => '0',
self::TYPE_BOOLEAN => 'false',
self::TYPE_OBJECT => '{}',
self::TYPE_STRING => "''",
+ default => "''",
};🤖 Prompt for AI Agents
In src/SDK/Language/Docs.php around lines 142 to 149, the match expression lacks
a default arm so unexpected $type values can raise an UnhandledMatchError; add a
fallback arm (e.g., default => 'null' or another appropriate example string) to
the match to return a safe default value for unrecognized types.
ChiragAgg5k
force-pushed
the
docs-sdk
branch
from
43cdc06 to
2bad05b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/SDK/Language/Docs.php (2)
137-144: Add default case to prevent UnhandledMatchError.This match expression will throw
UnhandledMatchErrorfor unexpected type values. As flagged in a previous review, add a default fallback.return match ($type) { self::TYPE_ARRAY => '[]', self::TYPE_FILE => 'file', self::TYPE_INTEGER, self::TYPE_NUMBER => '0', self::TYPE_BOOLEAN => 'false', self::TYPE_OBJECT => '{}', self::TYPE_STRING => "''", + default => "''", };
147-158: Add default case to prevent UnhandledMatchError.This match expression also lacks a default case and will throw for unexpected types. As flagged in a previous review, add a default fallback.
return match ($type) { self::TYPE_ARRAY => $this->isPermissionString($example) ? $this->getPermissionExample($example) : $example, self::TYPE_INTEGER, self::TYPE_NUMBER => (string)$example, self::TYPE_FILE => 'file', self::TYPE_BOOLEAN => ($example) ? 'true' : 'false', self::TYPE_OBJECT => ($example === '{}') ? '{}' : (($formatted = json_encode(json_decode($example, true), JSON_PRETTY_PRINT)) ? $formatted : $example), self::TYPE_STRING => "'{$example}'", + default => (string)$example, };
🧹 Nitpick comments (2)
example.php (1)
278-283: Consider consolidating duplicate catch blocks.Both catch blocks have identical handling logic. Since
ExceptionimplementsThrowable, you can simplify to a singleThrowablecatch to handle both exceptions and errors.-catch (Exception $exception) { - echo 'Error: ' . $exception->getMessage() . ' on ' . $exception->getFile() . ':' . $exception->getLine() . "\n"; -} -catch (Throwable $exception) { +catch (Throwable $exception) { echo 'Error: ' . $exception->getMessage() . ' on ' . $exception->getFile() . ':' . $exception->getLine() . "\n"; }src/SDK/Language/Docs.php (1)
10-10: Remove unused property.The
$paramsproperty is declared but never used in this class.class Docs extends Language { - protected $params = []; - /** * @return string */
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
example.php(2 hunks)src/SDK/Language/Docs.php(1 hunks)templates/docs/typescript/method.md.twig(1 hunks)
🧰 Additional context used
🪛 PHPMD (2.15.0)
src/SDK/Language/Docs.php
70-70: Avoid unused parameters such as '$spec'. (undefined)
(UnusedFormalParameter)
129-129: Avoid unused parameters such as '$lang'. (undefined)
(UnusedFormalParameter)
⏰ 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). (7)
- GitHub Check: build (8.3, KotlinJava11)
- GitHub Check: build (8.3, DotNet60)
- GitHub Check: build (8.3, Android14Java17)
- GitHub Check: go (server)
- GitHub Check: kotlin (server)
- GitHub Check: android (client)
- GitHub Check: apple (client)
🔇 Additional comments (9)
example.php (2)
24-24: LGTM!The import follows the existing convention used for other language implementations.
270-276: LGTM!The Docs SDK generation block follows the same pattern as other SDK generations (GraphQL, REST, etc.). Consistent with the existing codebase structure.
templates/docs/typescript/method.md.twig (1)
1-8: LGTM!The header section with deprecation handling is well-structured, providing clear visibility for deprecated methods with replacement suggestions.
src/SDK/Language/Docs.php (6)
15-50: LGTM!These accessor methods are well-implemented and follow TypeScript/JavaScript conventions appropriate for documentation generation.
55-64: LGTM!The file generation configuration correctly maps method-scoped templates to kebab-case filenames organized by service.
70-97: LGTM!The type resolution logic handles enums, arrays, files, objects, and primitives appropriately. The fallthrough to
return $parameter['type']on line 96 provides a sensible default for unrecognized types. The$specparameter is required by the parent signature.
103-122: LGTM!The default case in the match expression properly handles unknown types by returning
' = null'.
161-182: LGTM!The permission example rendering correctly transforms permission strings into the
Permission.Action(Role.Role(...))format with proper handling of optional id and innerRole parameters.
184-207: Consider addingparamExamplefilter if not inherited.The template uses
{{ parameter | paramExample }}but this filter isn't explicitly defined here. If the SDK framework doesn't automatically registergetParamExampleas a Twig filter, you'll need to add it:new TwigFilter('getResponseModel', function (array $method) { if (!empty($method['responseModel']) && $method['responseModel'] !== 'any') { return 'Models.' . \ucfirst($method['responseModel']); } return null; }), + new TwigFilter('paramExample', function ($value) { + return $this->getParamExample($value); + }),
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.