fix(zmodel): prefer to use triple-slash comments as ZModel documentation #1817
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce a new Changes
Assessment against linked issues
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (8)
packages/schema/src/language-server/zmodel-documentation-provider.ts (2)
8-8: Fix typo in the comment.
Change "prefer to user" to "prefer to use".
- // prefer to user triple-slash comments
+ // prefer to use triple-slash comments7-16: Add JSDoc documentation for the method.
The method override should include JSDoc documentation describing its purpose and return type.
Add this documentation above the method:
+ /**
+ * Retrieves documentation for an AST node, preferring triple-slash comments over JSDoc comments.
+ * @param node The AST node to get documentation for
+ * @returns The documentation string if found, undefined otherwise
+ */
getDocumentation(node: AstNode): string | undefined {packages/schema/src/language-server/zmodel-workspace-manager.ts (2)
Line range hint 44-57: Add defensive programming for plugin detection.
The plugin detection logic could be more robust by adding null checks and validation.
Consider applying these improvements:
documents.forEach((doc) => {
const parsed = doc.parseResult.value as Model;
- parsed.declarations.forEach((decl) => {
+ if (!parsed?.declarations) {
+ console.warn(`Invalid model structure in document: ${doc.uri}`);
+ return;
+ }
+ parsed.declarations.forEach((decl) => {
if (isPlugin(decl)) {
const providerField = decl.fields.find((f) => f.name === 'provider');
- if (providerField) {
+ if (providerField?.value) {
const provider = getLiteral<string>(providerField.value);
if (provider) {
this.pluginModels.add(provider);
+ } else {
+ console.warn(`Invalid provider value in plugin declaration: ${doc.uri}`);
}
}
}
});
});Line range hint 95-107: Improve stability of directory sorting.
The current sort function might lead to unstable sorting results. Non-node_modules directories always return 1, and non-directory entries return 0, which might not maintain their relative order.
Consider this improved implementation:
- ).sort((a, b) => {
- // make sure the node_moudules folder is always the first one to be checked
- // so it could be early exited if the plugin is found
- if (a.isDirectory && b.isDirectory) {
- const aName = Utils.basename(a.uri);
- if (aName === 'node_modules') {
- return -1;
- } else {
- return 1;
- }
- } else {
- return 0;
- }
+ ).sort((a, b) => {
+ const aName = Utils.basename(a.uri);
+ const bName = Utils.basename(b.uri);
+
+ // Prioritize directories over files
+ if (a.isDirectory !== b.isDirectory) {
+ return a.isDirectory ? -1 : 1;
+ }
+
+ // Among directories, prioritize node_modules
+ if (a.isDirectory && b.isDirectory) {
+ if (aName === 'node_modules') return -1;
+ if (bName === 'node_modules') return 1;
+ }
+
+ // Maintain stable order for everything else
+ return aName.localeCompare(bName);
});packages/schema/src/language-server/zmodel-module.ts (1)
81-83: LGTM: Documentation provider registration follows best practices.
The registration of ZModelDocumentationProvider is well-integrated into the module's dependency injection system and follows Langium's architectural patterns.
Consider documenting the documentation provider's capabilities in the module's JSDoc comments, similar to the "Declaration of custom services" comment above, to help other developers understand its role in the architecture.
packages/schema/src/plugins/prisma/schema-generator.ts (2)
288-291: LGTM! Consider extracting the comment formatting logic.
The implementation correctly handles non-Prisma attributes by converting them to triple-slash comments. The underscore wrapping ('/// - _' + ... + '_') helps distinguish attributes in the documentation.
Consider extracting the comment formatting logic into a helper function to improve reusability and maintain consistency:
+private formatAttributeComment(attr: DataModelAttribute): string {
+ return '/// - _' + this.zModelGenerator.generate(attr) + '_';
+}
+
decl.attributes
.filter((attr) => attr.decl.ref && !this.isPrismaAttribute(attr))
- .forEach((attr) => model.addComment('/// - _' + this.zModelGenerator.generate(attr) + '_'));
+ .forEach((attr) => model.addComment(this.formatAttributeComment(attr)));903-906: LGTM! Apply the same helper function here.
The implementation mirrors the model attribute handling, maintaining consistency in how non-Prisma attributes are converted to comments.
Use the same helper function suggested earlier to maintain consistency:
decl.attributes
.filter((attr) => attr.decl.ref && !this.isPrismaAttribute(attr))
- .forEach((attr) => _enum.addComment('/// - _' + this.zModelGenerator.generate(attr) + '_'));
+ .forEach((attr) => _enum.addComment(this.formatAttributeComment(attr)));packages/schema/tests/generator/prisma-generator.test.ts (1)
72-78: Consider using triple-slash comments for the Post model documentation
To maintain consistency with the preferred documentation style of triple-slash comments (///), consider updating the Post model's documentation accordingly.
Apply the following diff to update the documentation:
-/**
- * My post model
- * defined here
- */
+/// My post model
+/// defined here📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- packages/schema/src/language-server/zmodel-documentation-provider.ts (1 hunks)
- packages/schema/src/language-server/zmodel-module.ts (2 hunks)
- packages/schema/src/language-server/zmodel-workspace-manager.ts (1 hunks)
- packages/schema/src/plugins/prisma/schema-generator.ts (4 hunks)
- packages/schema/src/res/starter.zmodel (3 hunks)
- packages/schema/tests/generator/prisma-generator.test.ts (2 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/schema/src/res/starter.zmodel
🔇 Additional comments (9)
packages/schema/src/language-server/zmodel-documentation-provider.ts (2)
1-6: LGTM! Clean and well-documented class declaration.
The class is properly documented and follows a good inheritance pattern by extending JSDocDocumentationProvider.
1-16: Verify integration and existing usage.
Let's ensure this implementation works well with existing code.
✅ Verification successful
Let me check the module registration to ensure proper integration.
Implementation is properly integrated and working as expected
The verification shows:
- The documentation provider is correctly registered in the module system (
zmodel-module.ts) - Triple-slash comments are already in use across
.zmodelfiles with proper formatting - The implementation handles both documentation styles:
- Triple-slash comments (e.g.,
/// User model) - Falls back to JSDoc when triple-slash isn't present
- Triple-slash comments (e.g.,
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the registration and usage of the documentation provider
# Test 1: Check if the provider is properly registered in the module system
echo "Checking module registration..."
rg -l "ZModelDocumentationProvider" --type ts
# Test 2: Check existing triple-slash comment usage in .zmodel files
echo "Checking existing triple-slash comments in .zmodel files..."
rg "^///" --type-add 'zmodel:*.zmodel' --type zmodel -A 1
Length of output: 1583
Script:
#!/bin/bash
# Check how ZModelDocumentationProvider is registered in the module system
echo "Checking ZModelDocumentationProvider registration in zmodel-module.ts..."
rg -A 5 "ZModelDocumentationProvider" packages/schema/src/language-server/zmodel-module.ts
# Check if there are any other documentation providers or related configurations
echo "Checking for other documentation provider configurations..."
ast-grep --pattern 'createZModelServices($$$) {
$$$
}'
Length of output: 1033
packages/schema/src/language-server/zmodel-workspace-manager.ts (1)
12-12: Verify import statements across the codebase.
The change from default export to named export is a breaking change that requires updates to all importing files.
✅ Verification successful
Let me fix the regex syntax and try again to verify the imports.
Export change is safe and properly referenced
The change from default to named export is correctly handled. The class is only imported in one place (zmodel-module.ts) and it's already using the correct named import syntax. No other files in the codebase are affected by this change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all imports of ZModelWorkspaceManager to ensure they're updated
# Test: Search for both default and named import patterns
echo "Checking for potentially outdated default imports..."
rg "import\s+ZModelWorkspaceManager\s+from" -g '!*.{md,json}' || true
echo -e "\nChecking current named imports..."
rg "import\s*{\s*ZModelWorkspaceManager\s*}" -g '!*.{md,json}' || true
Length of output: 472
Script:
#!/bin/bash
# Description: Find all imports of ZModelWorkspaceManager to ensure they're updated
# Test: Search for both default and named import patterns
echo "Checking for potentially outdated default imports..."
rg "import ZModelWorkspaceManager from" -g '!*.{md,json}' || true
echo -e "\nChecking current named imports..."
rg "import.*\{.*ZModelWorkspaceManager.*\}" -g '!*.{md,json}' || true
# Also check for any other references to ZModelWorkspaceManager
echo -e "\nChecking all references to ZModelWorkspaceManager..."
rg "ZModelWorkspaceManager" -g '!*.{md,json}' || true
Length of output: 1008
packages/schema/src/language-server/zmodel-module.ts (1)
30-30: LGTM: Import changes are well-structured.
The new import for ZModelDocumentationProvider and the updated import style for ZModelWorkspaceManager are correctly placed and align with the module's organization.
Also applies to: 37-37
packages/schema/src/plugins/prisma/schema-generator.ts (2)
768-770: LGTM! Clean implementation of comment handling.
The code effectively combines user-defined comments with formatted attribute comments using array spread operators. This approach maintains a clean separation between different types of documentation while preserving all information.
Also applies to: 772-772
916-918: LGTM! Consistent with field comment handling.
The implementation maintains consistency with the field comment handling pattern, effectively combining user-defined comments with formatted attribute comments.
packages/schema/tests/generator/prisma-generator.test.ts (3)
50-58: LGTM!
The Role enum is correctly defined with appropriate documentation comments and the @@schema("auth") attribute.
60-70: LGTM!
The User model is properly documented and includes the necessary field and model attributes for access control and schema specification.
98-105: LGTM!
The test cases effectively verify the presence and absence of the expected documentation comments in the generated Prisma schema.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/schema/tests/generator/prisma-generator.test.ts (2)
99-106: Add assertion to verify JSDoc comment exclusion.
While the assertions effectively verify triple-slash comments, consider adding an assertion to explicitly verify that JSDoc comments (lines 72-75) are not included in the output. This would ensure the generator correctly differentiates between comment styles.
Add this assertion:
expect(content).not.toContain('* My post model');88-88: Add test case for customAttributesAsComments=false.
The customAttributesAsComments flag is consistently set to true across all test cases. Consider adding a test case that verifies the behavior when the flag is false to ensure complete coverage of this feature.
Add a new test case:
it('attributes not as comments', async () => {
// ... test setup with model containing attributes ...
await new PrismaSchemaGenerator(model).generate({
// ... other options ...
customAttributesAsComments: false,
});
const content = fs.readFileSync(name, 'utf-8');
expect(content).not.toContain('/// @allow');
expect(content).not.toContain('/// @deny');
});Also applies to: 209-209, 242-242, 436-436, 487-487, 519-519
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/schema/src/language-server/zmodel-documentation-provider.ts (1 hunks)
- packages/schema/src/plugins/prisma/schema-generator.ts (5 hunks)
- packages/schema/tests/generator/prisma-generator.test.ts (8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/schema/src/language-server/zmodel-documentation-provider.ts
🔇 Additional comments (3)
packages/schema/tests/generator/prisma-generator.test.ts (1)
50-78: LGTM! Comprehensive test data for documentation styles.
The test data effectively covers various documentation scenarios:
- Single-line and multi-line comments
- Model-level and field-level documentation
- Different comment styles (triple-slash and JSDoc)
packages/schema/src/plugins/prisma/schema-generator.ts (2)
103-103: LGTM: Property initialization and validation look good!
The implementation properly handles the new customAttributesAsComments option with appropriate type checking and validation.
Also applies to: 121-128
296-296: LGTM: Consistent comment handling implementation!
The changes maintain consistency in handling comments across different entity types (models, fields, enums, and enum fields) using a clean and immutable approach with the spread operator.
Also applies to: 772-774, 905-905, 913-915
fixes #1815 #1816