Parse project on target change #1488
Conversation
|
Hey @mdesmet, here is an example of how you can ask me to improve this pull request: @Sweep Add unit tests for `applySelectedTarget()` in `DBTCoreProjectIntegration` to verify that both `refreshProjectConfig()` and `rebuildManifest()` are called in the correct order. 📖 For more information on how to use Sweep, please read our documentation. |
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
You can disable this status message by setting the WalkthroughThe changes in this pull request introduce new asynchronous methods related to target selection and configuration management within the DBT integration framework. Specifically, the Changes
Possibly related PRs
Suggested reviewers
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 (
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 5614925 in 49 seconds
More details
- Looked at
76lines of code in4files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. src/dbt_client/dbtCloudIntegration.ts:1195
- Draft comment:
TheapplySelectedTargetmethod is implemented but not used. Consider removing it if it's not needed or ensure it's called where necessary. - Reason this comment was not posted:
Confidence changes required:50%
The methodapplySelectedTargetis implemented but not used in theDBTCloudProjectIntegrationclass. This might be an oversight or a placeholder for future functionality.
2. src/dbt_client/dbtCoreIntegration.ts:1206
- Draft comment:
TheapplySelectedTargetmethod is redundant as it only callsrefreshProjectConfigandrebuildManifest. Consider removing it and calling these methods directly where needed. - Reason this comment was not posted:
Confidence changes required:50%
TheapplySelectedTargetmethod inDBTCoreProjectIntegrationis redundant because it only calls two other methods that are already public and can be called directly. This method does not add any additional logic or abstraction.
3. src/dbt_client/dbtIntegration.ts:393
- Draft comment:
TheapplySelectedTargetmethod is added to theDBTProjectIntegrationinterface but is not used. Consider removing it if it's not needed or ensure it's implemented and used where necessary. - Reason this comment was not posted:
Confidence changes required:50%
TheapplySelectedTargetmethod is added to theDBTProjectIntegrationinterface but is not used in the codebase. This might be an oversight or a placeholder for future functionality.
Workflow ID: wflow_MrfvPfInUReoBxKT
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
src/dbt_client/dbtIntegration.ts (2)
393-393: Add documentation for the new method.
The purpose and behavior of applySelectedTarget() should be documented, particularly:
- Its relationship with
setSelectedTarget() - When it should be called
- The effects of applying a target
Consider adding JSDoc comments:
+ /**
+ * Applies the currently selected target to the project.
+ * This method should be called after setSelectedTarget() to [explain the effects].
+ * @returns A promise that resolves when the target has been applied
+ */
applySelectedTarget(): Promise<void>;393-393: Consider error handling and command queue integration.
The new method should:
- Follow the established error handling pattern used by other methods in the interface
- Consider integration with the command queue system in
DBTCommandExecutionInfrastructureif the operation is long-running
Example error handling pattern from existing code:
async applySelectedTarget(): Promise<void> {
try {
// Implementation
} catch (error) {
throw new DBTIntegrationError(`Failed to apply selected target: ${error.message}`);
}
}src/manifest/dbtProject.ts (1)
223-233: Consider adding error handling for target changes.
The progress notification implementation is good, but consider adding try-catch block to handle potential errors and provide user feedback.
async setSelectedTarget(targetName: string) {
await window.withProgress(
{
location: ProgressLocation.Notification,
title: "Changing target...",
cancellable: false,
},
async () => {
+ try {
await this.dbtProjectIntegration.setSelectedTarget(targetName);
await this.dbtProjectIntegration.applySelectedTarget();
+ window.showInformationMessage(`Successfully changed target to ${targetName}`);
+ } catch (error) {
+ window.showErrorMessage(`Failed to change target: ${error instanceof Error ? error.message : String(error)}`);
+ throw error; // Re-throw to maintain existing error propagation
+ }
},
);
}📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- src/dbt_client/dbtCloudIntegration.ts (1 hunks)
- src/dbt_client/dbtCoreIntegration.ts (1 hunks)
- src/dbt_client/dbtIntegration.ts (1 hunks)
- src/manifest/dbtProject.ts (2 hunks)
🔇 Additional comments (2)
src/dbt_client/dbtIntegration.ts (1)
393-393: Verify implementations in concrete classes.
The new interface method needs to be implemented by all classes that implement DBTProjectIntegration.
✅ Verification successful
Let me try a different search pattern to find the implementations, as the previous one might have been too strict.
Based on the search results, I can now generate the final response since we have found all the necessary implementations:
Implementation verified in both concrete classes
Both implementing classes have added the applySelectedTarget() method:
DBTCoreProjectIntegrationhas a full implementation that refreshes config and rebuilds manifestDBTCloudProjectIntegrationhas an empty implementation (which is valid since cloud integration may not need this functionality)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all classes that implement DBTProjectIntegration and verify they implement applySelectedTarget
# Search for class declarations that implement DBTProjectIntegration
echo "Classes implementing DBTProjectIntegration:"
ast-grep --pattern 'class $_ implements $$$DBTProjectIntegration$$$'
# Search for applySelectedTarget implementations
echo -e "\nImplementations of applySelectedTarget:"
ast-grep --pattern 'async applySelectedTarget() {
$$$
}'
Length of output: 323
Script:
#!/bin/bash
# Search for class declarations and their content
echo "Classes implementing DBTProjectIntegration:"
ast-grep --pattern 'class $_ implements DBTProjectIntegration {
$$$
}'
# Search for any mention of applySelectedTarget with more flexible pattern
echo -e "\nMentions of applySelectedTarget:"
rg "applySelectedTarget" -A 5
Length of output: 148518
src/manifest/dbtProject.ts (1)
15-15: LGTM: Import added for progress notification.
The addition of ProgressLocation import is appropriate for implementing the progress notification feature.
| await this.refreshProjectConfig(); | ||
| await this.rebuildManifest(); | ||
| } |
There was a problem hiding this comment.
Add error handling for refresh and rebuild operations.
The async operations should be wrapped in a try-catch block to handle potential errors consistently with other methods in the class.
Apply this diff to add error handling:
- await this.refreshProjectConfig();
- await this.rebuildManifest();
+ try {
+ await this.refreshProjectConfig();
+ await this.rebuildManifest();
+ } catch (error) {
+ this.dbtTerminal.error(
+ "dbtCoreIntegration:applyDeferConfig",
+ "An error occurred while refreshing config and rebuilding manifest",
+ error
+ );
+ throw error;
+ }📝 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.
| await this.refreshProjectConfig(); | |
| await this.rebuildManifest(); | |
| } | |
| try { | |
| await this.refreshProjectConfig(); | |
| await this.rebuildManifest(); | |
| } catch (error) { | |
| this.dbtTerminal.error( | |
| "dbtCoreIntegration:applyDeferConfig", | |
| "An error occurred while refreshing config and rebuilding manifest", | |
| error | |
| ); | |
| throw error; | |
| } | |
| } |
| async applySelectedTarget(): Promise<void> { | ||
| await this.refreshProjectConfig(); | ||
| await this.rebuildManifest(); | ||
| } |
There was a problem hiding this comment.
Improve method implementation with documentation and error handling.
The method should include JSDoc documentation and error handling to maintain consistency with other async methods in the class.
Apply this diff to improve the implementation:
+ /**
+ * Applies the selected target by refreshing project configuration and rebuilding manifest.
+ * @throws Error if refresh or rebuild operations fail
+ */
async applySelectedTarget(): Promise<void> {
- await this.refreshProjectConfig();
- await this.rebuildManifest();
+ try {
+ await this.refreshProjectConfig();
+ await this.rebuildManifest();
+ } catch (error) {
+ this.dbtTerminal.error(
+ "dbtCoreIntegration:applySelectedTarget",
+ "An error occurred while applying selected target",
+ error
+ );
+ throw error;
+ }
}📝 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.
| async applySelectedTarget(): Promise<void> { | |
| await this.refreshProjectConfig(); | |
| await this.rebuildManifest(); | |
| } | |
| /** | |
| * Applies the selected target by refreshing project configuration and rebuilding manifest. | |
| * @throws Error if refresh or rebuild operations fail | |
| */ | |
| async applySelectedTarget(): Promise<void> { | |
| try { | |
| await this.refreshProjectConfig(); | |
| await this.rebuildManifest(); | |
| } catch (error) { | |
| this.dbtTerminal.error( | |
| "dbtCoreIntegration:applySelectedTarget", | |
| "An error occurred while applying selected target", | |
| error | |
| ); | |
| throw error; | |
| } | |
| } |
| @@ -1192,6 +1192,8 @@ export class DBTCloudProjectIntegration | |||
|
|
|||
| async applyDeferConfig(): Promise<void> {} | |||
|
|
|||
| async applySelectedTarget(): Promise<void> {} | |||
There was a problem hiding this comment.
Implementation needed for target change handling.
The empty implementation of applySelectedTarget could lead to issues with project parsing when the target changes. Based on the PR objectives and the surrounding context, this method should handle the necessary steps to refresh the project configuration and rebuild the manifest when the target changes.
Consider implementing the method to:
- Refresh the project configuration
- Rebuild the manifest
- async applySelectedTarget(): Promise<void> {}
+ async applySelectedTarget(): Promise<void> {
+ await this.refreshProjectConfig();
+ await this.rebuildManifest();
+ }Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Skipped PR review on 07ec02c because no changed files had a supported extension. If you think this was in error, please contact us and we'll fix it right away.
Overview
Problem
Describe the problem you are solving. Mention the ticket/issue if applicable.
Solution
Describe the implemented solution. Add external references if needed.
Screenshot/Demo
A picture is worth a thousand words. Please highlight the changes if applicable.
How to test
Checklist
README.mdupdated and added information about my changeImportant
Add
applySelectedTarget()to handle target changes in DBT projects, ensuring config refresh and manifest rebuild.applySelectedTarget()method toDBTCloudProjectIntegrationandDBTCoreProjectIntegrationto refresh project config and rebuild manifest on target change.setSelectedTarget()inDBTProjectto callapplySelectedTarget()after setting the target.applySelectedTarget()toDBTProjectIntegrationinterface indbtIntegration.ts.setSelectedTarget()indbtProject.ts.This description was created by
for 5614925. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation