-
-
Notifications
You must be signed in to change notification settings - Fork 90
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
merge dev to main (v2.9.2) #1882
Conversation
📝 WalkthroughWalkthroughThis pull request includes updates across several files, primarily focusing on version increments and enhancements to type declarations and data model handling. The Changes
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/language/src/ast.ts (1)
61-65
: LGTM! Consider enhancing the documentation.The
$allFields
property is well-typed and follows the metadata naming convention.Consider adding an example in the documentation to clarify the difference between regular fields and those marked with
@ignore
:/** * All fields including those marked with `@ignore` + * @example + * // For a model with: + * model User { + * id String + * @ignore + * password String + * } + * // $allFields will contain both 'id' and 'password' fields */packages/schema/src/cli/cli-util.ts (1)
374-382
: Add documentation and consider defensive programmingThe function correctly implements field filtering for data models, but could benefit from some improvements:
- Add JSDoc documentation describing the function's purpose and side effects
- Add input validation for the model parameter
Consider applying these improvements:
+/** + * Filters out fields marked with @ignore attribute from data models while preserving + * all original fields in the $allFields property. + * @param model The model to process + * @throws {Error} If model is undefined or null + */ function filterIgnoredFields(model: Model) { + if (!model) { + throw new Error('Model parameter is required'); + } + model.declarations.forEach((decl) => { if (!isDataModel(decl)) { return; } decl.$allFields = [...decl.fields]; decl.fields = decl.fields.filter((f) => !hasAttribute(f, '@ignore')); }); }tests/integration/tests/plugins/zod.test.ts (1)
1085-1099
: LGTM! Well-structured test case for @ignore directive.The test case effectively verifies that fields marked with
@ignore
are properly excluded from schema validation. The assertions cover both the regular schema and Prisma create schema, ensuring comprehensive validation of the functionality.Consider adding a negative test case to verify that including the ignored field doesn't cause validation errors, as it would help document the expected behavior more completely:
expect(schemas.UserSchema.safeParse({ id: 1, email: '[email protected]' }).success).toBeTruthy(); +expect(schemas.UserSchema.safeParse({ id: 1, email: '[email protected]', password: 'secret' }).success).toBeTruthy(); expect(schemas.UserPrismaCreateSchema.safeParse({ email: '[email protected]' }).success).toBeTruthy(); +expect(schemas.UserPrismaCreateSchema.safeParse({ email: '[email protected]', password: 'secret' }).success).toBeTruthy();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (17)
package.json
is excluded by!**/*.json
packages/ide/jetbrains/package.json
is excluded by!**/*.json
packages/language/package.json
is excluded by!**/*.json
packages/misc/redwood/package.json
is excluded by!**/*.json
packages/plugins/openapi/package.json
is excluded by!**/*.json
packages/plugins/swr/package.json
is excluded by!**/*.json
packages/plugins/tanstack-query/package.json
is excluded by!**/*.json
packages/plugins/trpc/package.json
is excluded by!**/*.json
packages/plugins/trpc/tests/projects/nuxt-trpc-v10/package-lock.json
is excluded by!**/package-lock.json
,!**/*.json
packages/plugins/trpc/tests/projects/nuxt-trpc-v10/package.json
is excluded by!**/*.json
packages/plugins/trpc/tests/projects/nuxt-trpc-v11/package-lock.json
is excluded by!**/package-lock.json
,!**/*.json
packages/plugins/trpc/tests/projects/nuxt-trpc-v11/package.json
is excluded by!**/*.json
packages/runtime/package.json
is excluded by!**/*.json
packages/schema/package.json
is excluded by!**/*.json
packages/sdk/package.json
is excluded by!**/*.json
packages/server/package.json
is excluded by!**/*.json
packages/testtools/package.json
is excluded by!**/*.json
📒 Files selected for processing (6)
packages/ide/jetbrains/build.gradle.kts
(1 hunks)packages/language/src/ast.ts
(1 hunks)packages/schema/src/cli/cli-util.ts
(3 hunks)packages/schema/src/plugins/prisma/index.ts
(1 hunks)tests/integration/tests/plugins/zod.test.ts
(1 hunks)tests/integration/tests/schema/todo.zmodel
(5 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/ide/jetbrains/build.gradle.kts
- tests/integration/tests/schema/todo.zmodel
🔇 Additional comments (4)
packages/language/src/ast.ts (1)
Line range hint 8-17
: LGTM! Verify type resolution usage.
The new types ResolvedShape
and ResolvedType
are well-defined and documented.
Let's verify how these types are used in the codebase:
✅ Verification successful
LGTM! Type resolution system is well integrated.
The verification shows that the new types ResolvedShape
and ResolvedType
are extensively used throughout the codebase, particularly in:
- Type resolution during linking phase (
zmodel-linker.ts
) - Expression validation (
expression-validator.ts
) - Policy and constraint transformations
- OpenAPI schema generation
The implementation follows a consistent pattern where:
$resolvedType
is attached to expressions during linking- Validators check for type compatibility and constraints
- Code generators use the resolved type information for transformations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of the new type resolution system
# Expected: Find expressions where type resolution is performed
# Check for usage of $resolvedType
rg -t ts '\$resolvedType' -A 3
# Check for implementations handling ResolvedShape
ast-grep --pattern 'function $_($$_): ResolvedShape {
$$$
}'
Length of output: 30374
packages/schema/src/plugins/prisma/index.ts (1)
95-103
: LGTM! Improved path resolution for symlinked environments.
The changes correctly handle path resolution in symlinked environments (like pnpm) by first resolving @prisma/client
and then resolving the client files relative to that location. This is a robust solution that maintains compatibility across different package managers.
Let's verify the path resolution in a symlinked setup:
packages/schema/src/cli/cli-util.ts (2)
1-1
: LGTM!
The import of isDataModel
is appropriately placed with related AST imports and is used in the new functionality.
120-122
: LGTM!
The filterIgnoredFields
call is appropriately placed after model relinking, ensuring that ignored fields are filtered before the model is returned.
No description provided.