feat: sdk for md #1265
feat: sdk for md #1265
Conversation
WalkthroughAdds a new PHP SDK language class Appwrite\SDK\Language\Markdown implementing type inference, parameter defaults/examples, permission example rendering, Twig filters, and file output mappings. Adds a Markdown SDK template suite under templates/markdown (README, method template, package.json and package.json.twig, .gitignore, tsconfig, build script, package manifest), a TypeScript runtime SDK (DocsSDK, types, manifest, generated-types, index, generated-types), a scripts/build-manifest.ts utility to generate a docs manifest and types, and a GitHub Actions publish workflow. Integrates Markdown generation into example.php's configure flow and expands error handling to catch Throwable in addition to Exception. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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.
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); + }),
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @src/SDK/Language/Markdown.php:
- Around line 187-189: The getPropertyType Twig filter passes a second argument
named $method into getTypeName(), but getTypeName() currently declares the
parameter as $spec and never uses it, causing a confusing mismatch; fix this by
renaming the second parameter in getTypeName() from $spec to $method (and update
its PHPDoc) so the signature matches how templates call it (getPropertyType ->
getTypeName($value, $method)), ensuring no behavior change and clarifying
intent; verify and adjust any references to the parameter name inside the
getTypeName() method or its callers if present.
♻️ Duplicate comments (2)
src/SDK/Language/Markdown.php (2)
137-144: Add default case to prevent UnhandledMatchError.This match expression lacks a default arm. If
$typecontains an unexpected value, anUnhandledMatchErrorwill be thrown. This issue was previously flagged but remains unaddressed.🔎 Proposed fix
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 arm and can throw
UnhandledMatchErrorfor unexpected$typevalues. This issue was previously flagged but remains unaddressed.🔎 Proposed fix
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 (5)
templates/markdown/typescript/method.md.twig (3)
15-16: Consider breaking down the complex parameter table row.Line 16 combines parameter information, default values, and enum values into a single table cell with multiple conditionals. While functional, this makes the template harder to maintain and debug.
🔎 Potential refactor for improved readability
Consider extracting the description cell content into a separate macro or breaking it into multiple lines with Twig's whitespace control:
-| `{{ parameter.name | caseCamel }}` | `{{ parameter | getPropertyType }}` | {% if parameter.required %}✅{% else %}❌{% endif %} | {{ parameter.description | raw }}{% if parameter.default %} (Default: `{{ parameter.default }}`){% endif %}{% if parameter.enumValues | length > 0 %}<br>**Allowed:** {% for value in parameter.enumValues %}`{{ value }}`{% if not loop.last %}, {% endif %}{% endfor %}{% endif %} | +| `{{ parameter.name | caseCamel }}` | `{{ parameter | getPropertyType }}` | {% if parameter.required %}✅{% else %}❌{% endif %} | + {{- parameter.description | raw -}} + {%- if parameter.default %} (Default: `{{ parameter.default }}`){% endif -%} + {%- if parameter.enumValues | length > 0 %}<br>**Allowed:** {% for value in parameter.enumValues %}`{{ value }}`{% if not loop.last %}, {% endif %}{% endfor %}{% endif %} |
26-26: Simplify the complex import statement.The import line combines multiple nested loops and conditionals, making it difficult to read and maintain. The logic chains together service imports, enum imports, and Models import with complex conditions.
🔎 Proposed refactor to improve readability
Consider breaking the imports into separate lines or using Twig's set tag to build the import list:
{% set imports = [service.name | caseUcfirst] %} {% for parameter in method.parameters.all %} {% if parameter.enumValues | length > 0 %} {% set imports = imports|merge([parameter.enumName | caseUcfirst]) %} {% endif %} {% endfor %} {% if method | getResponseModel %} {% set imports = imports|merge(['Models']) %} {% endif %} import { Client, {{ imports|join(', ') }} } from '{{ spec.title | caseLower }}';
40-67: Reduce code duplication in method invocation blocks.The three conditional branches (location, webAuth, default) contain nearly identical parameter rendering logic (lines 43-46, 52-55, 61-64), differing only in method signature details. This duplication increases maintenance burden and the risk of inconsistencies.
🔎 Proposed refactor using a macro
Consider extracting the parameter rendering into a Twig macro:
{% macro renderParams(method, service, parameter) %} {% for parameter in method.parameters.all %} {{ parameter.name | caseCamel }}: {% if parameter.enumValues | length > 0 %}{{ parameter.enumName | caseUcfirst }}.{{ (parameter.enumKeys[0] ?? parameter.enumValues[0]) | caseEnumKey }}{% else %}{{ parameter | paramExample }}{% endif %}, {% endfor %} {% endmacro %} {% import _self as macros %} {% if method.type == 'location' %} const result{% if method | getResponseModel %}: {{ method | getResponseModel }}{% endif %} = {{ service.name | caseCamel }}.{{ method.name | caseCamel }}({% if method.parameters.all | length == 0 %}); {% else %}{ {{ macros.renderParams(method, service, parameter) -}} }); {% endif %} {# ... similar for other branches #}src/SDK/Language/Markdown.php (2)
10-10: Remove unused property.The
$paramsproperty is declared but never used in this class. Consider removing it to reduce clutter.🔎 Proposed cleanup
- protected $params = []; - /** * @return string */
152-156: Simplify the complex JSON formatting logic.The nested ternary with inline assignment makes this code difficult to read and maintain. Consider extracting this logic for clarity.
🔎 Proposed refactor
- self::TYPE_OBJECT => ($example === '{}') - ? '{}' - : (($formatted = json_encode(json_decode($example, true), JSON_PRETTY_PRINT)) - ? $formatted - : $example), + self::TYPE_OBJECT => $this->formatObjectExample($example),And add a helper method:
private function formatObjectExample(string $example): string { if ($example === '{}') { return '{}'; } $decoded = json_decode($example, true); if ($decoded === null) { return $example; } $formatted = json_encode($decoded, JSON_PRETTY_PRINT); return $formatted ?: $example; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
example.phpsrc/SDK/Language/Markdown.phptemplates/markdown/typescript/method.md.twig
🚧 Files skipped from review as they are similar to previous changes (1)
- example.php
🧰 Additional context used
🧬 Code graph analysis (1)
src/SDK/Language/Markdown.php (1)
src/SDK/Language.php (5)
Language(5-295)isPermissionString(176-180)extractPermissionParts(182-219)toCamelCase(121-136)toPascalCase(116-119)
🪛 PHPMD (2.15.0)
src/SDK/Language/Markdown.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). (20)
- GitHub Check: build (8.3, Ruby27)
- GitHub Check: build (8.3, AppleSwift56)
- GitHub Check: build (8.3, Python310)
- GitHub Check: build (8.3, Go112)
- GitHub Check: build (8.3, KotlinJava11)
- GitHub Check: build (8.3, DartStable)
- GitHub Check: build (8.3, KotlinJava17)
- GitHub Check: build (8.3, Node18)
- GitHub Check: build (8.3, Go118)
- GitHub Check: build (8.3, CLINode18)
- GitHub Check: build (8.3, DotNet80)
- GitHub Check: build (8.3, FlutterBeta)
- GitHub Check: build (8.3, KotlinJava8)
- GitHub Check: build (8.3, DartBeta)
- GitHub Check: build (8.3, Android14Java17)
- GitHub Check: kotlin (server)
- GitHub Check: swift (server)
- GitHub Check: flutter (client)
- GitHub Check: android (client)
- GitHub Check: apple (client)
🔇 Additional comments (9)
templates/markdown/typescript/method.md.twig (2)
1-8: LGTM!The header section correctly renders the method name, deprecation warning (when applicable), and description using appropriate filters.
69-82: LGTM!The response model section correctly renders the model properties when available, using appropriate conditionals and filters.
src/SDK/Language/Markdown.php (7)
15-50: LGTM!The metadata methods correctly implement the Language interface with appropriate values for Markdown/TypeScript documentation generation.
55-64: LGTM!The file generation configuration correctly maps to the Markdown template and uses appropriate filters for the destination path.
70-97: LGTM!The type name conversion logic correctly handles enums, arrays (including nested arrays), and primitive types for TypeScript-like output. The unused
$specparameter is required by the parent abstract method signature.
103-122: LGTM!The parameter default value logic is correct and includes a proper default case in the match expression to handle unexpected types.
161-182: LGTM!The permission example generation correctly handles various permission structures (with/without IDs and inner roles) and produces TypeScript-compatible output.
190-196: LGTM!The comment filter appropriately handles multi-line text with word wrapping and is correctly marked as HTML-safe.
197-205: LGTM!The caseEnumKey and getResponseModel filters correctly implement their respective formatting logic for enum keys and response model names.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/SDK/Language/Markdown.php (3)
70-97: Unused$specparameter is acceptable if required by parent signature.The switch statement correctly falls through to line 96 which returns
$parameter['type']as the default behavior. However, the unused$specparameter should be verified - if it's required by the parent class's abstract method signature, keep it; otherwise, consider removing it.#!/bin/bash # Verify if getTypeName has an abstract signature in the parent class rg -n "function getTypeName" src/SDK/Language.php -A 3
184-221: Twig filters are well-implemented.The filters provide useful utilities for the Markdown templates. The
commentfilter's 80-character word wrap is a nice touch for documentation formatting.Note: The
getPropertyTypefilter passes$methodas the second argument togetTypeName(), which declares it as$spec. This naming mismatch was previously flagged but doesn't affect functionality since the parameter is unused.
137-158: Missing default cases in match expressions can throwUnhandledMatchError.Both match expressions lack default cases. If
$typecontains an unexpected value, anUnhandledMatchErrorwill be thrown at runtime.🔎 Proposed fix
if (!$hasExample) { 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 => "''", }; } 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 => "'{$example}'", };
🧹 Nitpick comments (1)
src/SDK/Language/Markdown.php (1)
10-10: Unused property$params.This property is declared but never referenced in the class. Consider removing it if not needed, or document its intended purpose.
🔎 Proposed fix
class Markdown extends Language { - protected $params = []; - /** * @return string */
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/SDK/Language/Markdown.phptemplates/markdown/typescript/method.md.twig
🚧 Files skipped from review as they are similar to previous changes (1)
- templates/markdown/typescript/method.md.twig
🧰 Additional context used
🧬 Code graph analysis (1)
src/SDK/Language/Markdown.php (1)
src/SDK/Language.php (5)
Language(5-295)isPermissionString(176-180)extractPermissionParts(182-219)toCamelCase(121-136)toPascalCase(116-119)
🪛 PHPMD (2.15.0)
src/SDK/Language/Markdown.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). (19)
- GitHub Check: build (8.3, Python311)
- GitHub Check: build (8.3, WebNode)
- GitHub Check: build (8.3, Python313)
- GitHub Check: build (8.3, AppleSwift56)
- GitHub Check: build (8.3, Ruby27)
- GitHub Check: build (8.3, KotlinJava17)
- GitHub Check: build (8.3, Node20)
- GitHub Check: build (8.3, KotlinJava8)
- GitHub Check: build (8.3, DartStable)
- GitHub Check: build (8.3, KotlinJava11)
- GitHub Check: build (8.3, DotNet90)
- GitHub Check: build (8.3, FlutterBeta)
- GitHub Check: build (8.3, Android5Java17)
- GitHub Check: build (8.3, Android14Java17)
- GitHub Check: kotlin (server)
- GitHub Check: swift (server)
- GitHub Check: dotnet (server)
- GitHub Check: apple (client)
- GitHub Check: android (client)
🔇 Additional comments (4)
src/SDK/Language/Markdown.php (4)
15-50: LGTM!The metadata methods are correctly implemented with appropriate return values for TypeScript-like Markdown documentation generation.
55-64: LGTM!The file mapping configuration correctly defines the template and destination path structure for method-level Markdown documentation.
103-122: LGTM!The method correctly handles required parameters, explicit defaults, and provides sensible fallbacks for different types including a default case.
161-182: LGTM!The permission example generation correctly parses permission strings and formats them as
Permission.Action(Role.Role(...))calls, properly handling the optionalidandinnerRolecomponents.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@templates/markdown/package.json.twig`:
- Around line 37-41: Update the pinned devDependency versions in
templates/markdown/package.json.twig: replace the values for "@types/node",
"tsx", and "typescript" with their current releases (e.g. `@types/node` → 25.0.9,
tsx → 4.21.0, typescript → 5.9.3) to bring them up to date; after changing the
version strings for the "@types/node", "tsx", and "typescript" entries,
regenerate the lockfile (npm/yarn/pnpm install) and run the project's build/type
checks to verify compatibility.
In `@templates/markdown/scripts/build-manifest.ts`:
- Around line 158-175: The generated MethodPaths interface uses raw language
names as object keys which breaks TypeScript for names with hyphens (e.g.,
react-native); update the methodPathsEntries generation (the languages,
manifest, methodPathsEntries block) to quote and escape each language key when
building the entry (e.g., produce "'lang-name': ..." instead of "lang-name:
..."), ensuring any single quotes in the language string are escaped so the
emitted TypeScript remains syntactically valid.
In `@templates/markdown/src/index.ts`:
- Around line 250-257: The code constructs a RegExp from user-controlled term
(new RegExp(term, 'gi')) which can lead to ReDoS; fix by escaping regex
metacharacters before building the RegExp (e.g., add an escapeRegExp utility and
call it on term) and/or wrap RegExp construction in a try/catch to safely handle
malformed patterns, then use the escaped pattern when creating the regex used
against lowerContent (affecting the variables term, regex, matches, score, and
matchedTerms).
♻️ Duplicate comments (2)
src/SDK/Language/Markdown.php (2)
163-198: Normalize non-string examples and add match fallbacks.
If$exampleis an array (common after decoding JSON examples),isPermissionString()will throw aTypeError, and the function may return a non-string despite the declared return type. Also, bothmatchblocks lack adefault, which can throwUnhandledMatchErrorfor unexpected types.🛠️ Proposed fix
- $example = $param['example'] ?? ''; + $example = $param['example'] ?? ''; + if (is_array($example)) { + $example = json_encode($example, JSON_PRETTY_PRINT) ?: '[]'; + } @@ if (!$hasExample) { 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 => "''", }; } @@ 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, };
138-160: Fix incorrect default value handling for falsy values in getParamDefault().The current implementation uses
!empty($default), which incorrectly drops valid falsy defaults like0,false,"0", and"". This produces incorrect parameter defaults in generated documentation.This is inconsistent with other Language implementations (e.g., REST.php and JS.php), which explicitly handle these cases with
empty($default) && $default !== 0 && $default !== false. Additionally, type-aware formatting for strings and booleans is missing, causing unquoted strings and boolean values to be rendered incorrectly.🛠️ Proposed fix
- if (!empty($default)) { - return ' = ' . $default; - } - - return match ($type) { + $hasDefault = array_key_exists('default', $param); + + if ($hasDefault) { + return match ($type) { + self::TYPE_BOOLEAN => ' = ' . ($default ? 'true' : 'false'), + self::TYPE_STRING => " = '{$default}'", + self::TYPE_ARRAY, + self::TYPE_OBJECT, + self::TYPE_INTEGER, + self::TYPE_NUMBER => ' = ' . (string)$default, + default => ' = ' . (string)$default, + }; + } + + return match ($type) { self::TYPE_ARRAY => ' = []', self::TYPE_OBJECT => ' = {}', default => ' = null', };
🧹 Nitpick comments (3)
templates/markdown/scripts/build-manifest.ts (1)
36-69: Consider edge cases in metadata extraction.The
extractMetadatafunction has a few considerations:
Line 48: The deprecation check uses a hardcoded string
'> ⚠️ This method is deprecated'. If the format varies slightly in generated docs, this could miss deprecation notices.Lines 58-64: The description extraction skips lines starting with
>(blockquotes) which is good, but doesn't skip code blocks (``` sections), which could inadvertently pick up code as description.♻️ Optional: More robust description extraction
function extractMetadata(content: string): { title: string; description: string; deprecated: boolean; } { const lines = content.split('\n'); // Extract title from first H1 const titleLine = lines.find((line) => line.startsWith('# ')); const title = titleLine ? titleLine.replace(/^#\s+/, '').trim() : ''; - // Check for deprecation notice - const deprecated = content.includes('> ⚠️ This method is deprecated'); + // Check for deprecation notice (case-insensitive, flexible format) + const deprecated = />\s*⚠️?\s*.*deprecated/i.test(content); // Extract description - look for first paragraph after title let description = ''; let foundTitle = false; + let inCodeBlock = false; for (const line of lines) { + if (line.startsWith('```')) { + inCodeBlock = !inCodeBlock; + continue; + } + if (inCodeBlock) continue; if (line.startsWith('# ')) { foundTitle = true; continue; } if (foundTitle && line.trim() && !line.startsWith('#') && !line.startsWith('>')) { description = line .replace(/\*\*/g, '') .replace(/`/g, '') .trim() .slice(0, 200); break; } } return { title, description, deprecated }; }templates/markdown/src/index.ts (2)
259-265: Scoring logic may not correctly detect "all terms matched".The condition
matchedTerms === queryTerms.length * 2assumes each term is counted twice (once for title, once for content). However, if a term appears in title but not content (or vice versa),matchedTermswon't reachqueryTerms.length * 2, even though all terms were found in at least one location.Consider whether the intent is "all terms found in both title AND content" or "all terms found in at least one place."
157-165: Promise.all with map mutation may have race condition.Using
Promise.allwith callbacks that mutate a sharedMapis generally safe in JavaScript/Node.js since the event loop is single-threaded. However, the pattern could be clearer by returning values and constructing the Map afterward.♻️ Optional: Cleaner async pattern
- await Promise.all( - serviceEntry.methods.map(async (method) => { - const path = `${service}/${method.name}` as MethodPaths[L]; - const content = await this.getMarkdown(language, path); - if (content) { - result.set(method.name, content); - } - }) - ); + const entries = await Promise.all( + serviceEntry.methods.map(async (method) => { + const path = `${service}/${method.name}` as MethodPaths[L]; + const content = await this.getMarkdown(language, path); + return content ? [method.name, content] as const : null; + }) + ); + + for (const entry of entries) { + if (entry) result.set(entry[0], entry[1]); + }
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/SDK/Language/Markdown.php`:
- Around line 53-107: The comment for the manifest entry in getFiles() is
inaccurate — the manifest template 'markdown/src/manifest.ts' is listed with
'scope' => 'copy' and contains no Twig placeholders, so update the comment near
the manifest array item (the entry that sets 'destination' => 'src/manifest.ts'
and 'template' => 'markdown/src/manifest.ts') to reflect that this file is
copied as-is (e.g., remove “needs templating for TOC generation” or replace it
with “Manifest (static - just copy)”) so the comment matches the 'scope' =>
'copy' behavior.
- Around line 148-167: The getParamDefault method incorrectly uses an empty()
check that drops valid falsy defaults like 0, false, and '' — update the logic
to detect whether a default was provided by checking the presence of the key
instead of emptiness: replace the current "!empty($default)" condition with a
key-existence check (e.g., array_key_exists('default', $param)) so
getParamDefault() will return ' = ' . $default for provided falsy values while
preserving the required/typed fallbacks.
♻️ Duplicate comments (1)
src/SDK/Language/Markdown.php (1)
174-204: Missing default arm in match (duplicate).The match expressions can throw
UnhandledMatchErrorfor unexpected types; please add a default fallback. Marking as duplicate of the earlier review.
🧹 Nitpick comments (1)
src/SDK/Language/Markdown.php (1)
115-142: Suppress PHPMD unused-parameter warning.
$methodis required by call sites but unused here; add a suppression to avoid recurring lint noise.♻️ Proposed suppression
/** * `@param` array $parameter * `@param` array $method * `@return` string + * `@SuppressWarnings`(PHPMD.UnusedFormalParameter) */
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/SDK/Language/Markdown.php`:
- Around line 118-141: The getTypeName logic in Markdown.php collapses
parameters with a "model" or "array.model" to 'object' or 'any[]'; update the
method (getTypeName) to check for $parameter['model'] and for
($parameter['array']['model'] ?? null) before falling back to
TYPE_OBJECT/TYPE_ARRAY: if a model is present return
ucfirst($parameter['model']); for arrays, if ($parameter['array']['model'])
return ucfirst($parameter['array']['model']) . '[]' (or call $this->getTypeName
on the array element and append '[]' to mirror enumName handling), and preserve
the existing items -> array mapping behavior so model-backed types render their
model names instead of object/any[].
♻️ Duplicate comments (1)
src/SDK/Language/Markdown.php (1)
181-203: Add default arms to bothmatchexpressions to avoidUnhandledMatchError.Unknown or custom
$typevalues will currently throw at runtime. Adding a fallback keeps doc generation resilient. (Similar to prior reviews on match defaults.)✅ Suggested fix
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 => 'null', }; } 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, };
| if (isset($parameter['enumName'])) { | ||
| return \ucfirst($parameter['enumName']); | ||
| } | ||
| if (!empty($parameter['enumValues'])) { | ||
| return \ucfirst($parameter['name']); | ||
| } | ||
| if (isset($parameter['items'])) { | ||
| $parameter['array'] = $parameter['items']; | ||
| } | ||
| switch ($parameter['type']) { | ||
| case self::TYPE_INTEGER: | ||
| case self::TYPE_NUMBER: | ||
| return 'number'; | ||
| case self::TYPE_ARRAY: | ||
| if (!empty(($parameter['array'] ?? [])['type']) && !\is_array($parameter['array']['type'])) { | ||
| return $this->getTypeName($parameter['array']) . '[]'; | ||
| } | ||
| return 'any[]'; | ||
| case self::TYPE_FILE: | ||
| return 'File'; | ||
| case self::TYPE_OBJECT: | ||
| return 'object'; | ||
| } | ||
| return $parameter['type']; |
There was a problem hiding this comment.
Handle model / array.model to avoid collapsing types to object/any[].
If parameters include model or array.model, the current logic falls through to object/any[], losing model names in generated docs. Consider mirroring the base JS behavior so model-backed types are rendered correctly.
✅ Suggested fix
if (isset($parameter['items'])) {
$parameter['array'] = $parameter['items'];
}
+ if (!empty($parameter['array']['model'])) {
+ return $this->toPascalCase($parameter['array']['model']) . '[]';
+ }
+ if (!empty($parameter['model'])) {
+ $modelType = $this->toPascalCase($parameter['model']);
+ return $parameter['type'] === self::TYPE_ARRAY ? $modelType . '[]' : $modelType;
+ }
switch ($parameter['type']) {🤖 Prompt for AI Agents
In `@src/SDK/Language/Markdown.php` around lines 118 - 141, The getTypeName logic
in Markdown.php collapses parameters with a "model" or "array.model" to 'object'
or 'any[]'; update the method (getTypeName) to check for $parameter['model'] and
for ($parameter['array']['model'] ?? null) before falling back to
TYPE_OBJECT/TYPE_ARRAY: if a model is present return
ucfirst($parameter['model']); for arrays, if ($parameter['array']['model'])
return ucfirst($parameter['array']['model']) . '[]' (or call $this->getTypeName
on the array element and append '[]' to mirror enumName handling), and preserve
the existing items -> array mapping behavior so model-backed types render their
model names instead of object/any[].
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/SDK/Language/Markdown.php`:
- Around line 158-160: The concatenation that returns ' = ' . $default when
array_key_exists('default', $param) produces incorrect representations for
non-string types; update the default formatting to be type-aware by converting
$default to a proper string representation (e.g. use var_export($default, true)
or a similar serializer) before concatenation so booleans become "true"/"false",
null becomes "NULL"/"null", and arrays are rendered sensibly; locate the
conditional around array_key_exists('default', $param) in Markdown.php and
replace the direct concatenation with a call that formats $default appropriately
(ensuring strings remain quoted if desired).
♻️ Duplicate comments (3)
src/SDK/Language/Markdown.php (3)
118-141: Handlemodeltypes to preserve type information in generated docs.Parameters with
modelorarray.modelcurrently fall through toobject/any[], losing model names in generated documentation. This was flagged in a previous review and remains unaddressed.Suggested fix
if (isset($parameter['items'])) { $parameter['array'] = $parameter['items']; } + if (!empty($parameter['array']['model'])) { + return $this->toPascalCase($parameter['array']['model']) . '[]'; + } + if (!empty($parameter['model'])) { + return $this->toPascalCase($parameter['model']); + } switch ($parameter['type']) {
182-189: Missing default case in match expression.If
$typeis an unexpected value, this match expression will throw anUnhandledMatchError. This was flagged in a previous review.Proposed fix
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 => "''", };
192-203: Missing default case in match expression.The second match expression also lacks a default case, risking
UnhandledMatchErrorfor unexpected types. This was flagged in a previous review.Proposed fix
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, };
| if (array_key_exists('default', $param)) { | ||
| return ' = ' . $default; | ||
| } |
There was a problem hiding this comment.
Default value formatting is broken for non-string types.
Direct string concatenation on line 159 doesn't properly format non-string defaults:
- Boolean
falseproduces' = 'instead of' = false' - Boolean
trueproduces' = 1'instead of' = true' - Arrays produce
' = Array'
Proposed fix with type-aware formatting
if (array_key_exists('default', $param)) {
- return ' = ' . $default;
+ return ' = ' . match ($type) {
+ self::TYPE_BOOLEAN => $default ? 'true' : 'false',
+ self::TYPE_ARRAY => json_encode($default),
+ self::TYPE_OBJECT => json_encode($default),
+ self::TYPE_STRING => "'{$default}'",
+ default => (string)$default,
+ };
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (array_key_exists('default', $param)) { | |
| return ' = ' . $default; | |
| } | |
| if (array_key_exists('default', $param)) { | |
| return ' = ' . match ($type) { | |
| self::TYPE_BOOLEAN => $default ? 'true' : 'false', | |
| self::TYPE_ARRAY => json_encode($default), | |
| self::TYPE_OBJECT => json_encode($default), | |
| self::TYPE_STRING => "'{$default}'", | |
| default => (string)$default, | |
| }; | |
| } |
🤖 Prompt for AI Agents
In `@src/SDK/Language/Markdown.php` around lines 158 - 160, The concatenation that
returns ' = ' . $default when array_key_exists('default', $param) produces
incorrect representations for non-string types; update the default formatting to
be type-aware by converting $default to a proper string representation (e.g. use
var_export($default, true) or a similar serializer) before concatenation so
booleans become "true"/"false", null becomes "NULL"/"null", and arrays are
rendered sensibly; locate the conditional around array_key_exists('default',
$param) in Markdown.php and replace the direct concatenation with a call that
formats $default appropriately (ensuring strings remain quoted if desired).
Summary by CodeRabbit
New Features
Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.