chore: unify the logic of saving ts files #1874
Conversation
📝 WalkthroughWalkthroughThe pull request introduces modifications to 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 (
|
![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
🧹 Outside diff range and nitpick comments (1)
packages/sdk/src/model-meta-generator.ts (1)
Inconsistent file saving patterns detected across the codebase
The verification reveals mixed usage of file saving methods that need to be unified:
- Direct
.save()calls are still present in multiple files, particularly in error handling paths incode-gen.tssaveSourceFileimplementation incode-gen.tsappears to be the standardized approach, but it's not consistently used- Some files like
enhance/index.tshave both patternsKey locations requiring attention:
packages/schema/src/plugins/enhancer/enhance/index.ts: Contains bothsave()andsaveSourceFileusagepackages/sdk/src/code-gen.ts: Has multipleproject.save()calls in error handling pathspackages/schema/tests/generator/expression-writer.test.ts: Uses directsave()callpackages/misc/redwood/src/commands/setup.ts: Uses directproject.save()call🔗 Analysis chain
Line range hint
70-88: LGTM: Function changes align with unified file saving approachThe transition from async to sync execution and the use of
saveSourceFileis consistent with the PR's objective of unifying TypeScript file saving logic.Let's verify the consistent usage of
saveSourceFileacross the codebase:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent usage of saveSourceFile and absence of direct .save() calls # Check for any remaining direct .save() calls rg -l "\.save\(\)" --type ts # Check for consistent usage of saveSourceFile rg -l "saveSourceFile\(" --type tsLength of output: 446
Script:
#!/bin/bash # Let's examine the actual usage context of .save() and saveSourceFile in these files # Check .save() usage context echo "=== .save() usage in enhance/index.ts ===" rg "\.save\(\)" packages/schema/src/plugins/enhancer/enhance/index.ts -B 2 -A 2 echo -e "\n=== .save() usage in code-gen.ts ===" rg "\.save\(\)" packages/sdk/src/code-gen.ts -B 2 -A 2 echo -e "\n=== .save() usage in expression-writer.test.ts ===" rg "\.save\(\)" packages/schema/tests/generator/expression-writer.test.ts -B 2 -A 2 echo -e "\n=== .save() usage in setup.ts ===" rg "\.save\(\)" packages/misc/redwood/src/commands/setup.ts -B 2 -A 2 # Check saveSourceFile usage context echo -e "\n=== saveSourceFile usage in code-gen.ts ===" rg "saveSourceFile\(" packages/sdk/src/code-gen.ts -B 2 -A 2 echo -e "\n=== saveSourceFile usage in enhance/index.ts ===" rg "saveSourceFile\(" packages/schema/src/plugins/enhancer/enhance/index.ts -B 2 -A 2 echo -e "\n=== saveSourceFile usage in policy-guard-generator.ts ===" rg "saveSourceFile\(" packages/schema/src/plugins/enhancer/policy/policy-guard-generator.ts -B 2 -A 2Length of output: 2943
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/schema/src/plugins/enhancer/enhance/index.ts(3 hunks)packages/schema/src/plugins/enhancer/policy/policy-guard-generator.ts(3 hunks)packages/sdk/src/model-meta-generator.ts(3 hunks)
🔇 Additional comments (5)
packages/sdk/src/model-meta-generator.ts (1)
41-41: LGTM: Import addition aligns with unified file saving approach
The addition of saveSourceFile import is consistent with the PR's objective of unifying TypeScript file saving logic.
packages/schema/src/plugins/enhancer/policy/policy-guard-generator.ts (2)
29-29: LGTM: Import of saveSourceFile is properly placed
The addition of saveSourceFile to the existing imports from @zenstackhq/sdk is well-organized and aligns with the PR's objective of unifying the ts file saving logic.
90-90: Consider error handling for synchronous file saving
With the transition to synchronous file saving, ensure that potential file system errors are properly handled. The saveSourceFile function should include appropriate error handling mechanisms.
#!/bin/bash
# Check implementation of saveSourceFile for error handling
rg -A 5 "export function saveSourceFile"packages/schema/src/plugins/enhancer/enhance/index.ts (2)
14-14: LGTM: Import statement added correctly.
The import of saveSourceFile from '@zenstackhq/sdk' aligns with the transition to synchronous file operations.
141-141: Verify the removal of await.
The change from await this.saveSourceFile(enhanceTs) to this.saveSourceFile(enhanceTs) aligns with making the operation synchronous. However, we should verify that this doesn't affect error handling.
✅ Verification successful
Removal of await is correct and safe
The verification shows that saveSourceFile is a synchronous function that performs file text replacement operations. Looking at its implementation in packages/sdk/src/code-gen.ts, it doesn't contain any asynchronous operations. The function is consistently used synchronously across the codebase, including in other plugins. The removal of await aligns with its synchronous nature and won't affect error handling since there were no try-catch blocks found around its usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any error handling related to saveSourceFile
rg -A 5 "saveSourceFile.*catch"
# Search for any async/await usage patterns with saveSourceFile
rg -A 5 "await.*saveSourceFile"
Length of output: 67
Script:
#!/bin/bash
# Let's first check the implementation of saveSourceFile
ast-grep --pattern 'saveSourceFile($_) { $$$ }'
# Also check if there are any try-catch blocks around saveSourceFile calls
ast-grep --pattern 'try {
$$$
saveSourceFile($_)
$$$
}'
# Let's see the method declaration and its usage context
rg -B 5 -A 5 "saveSourceFile"
Length of output: 9540
No description provided.