Skip to content
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.

Sign up for GitHub

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

Parse project on target change #1488

Merged
merged 2 commits into from
Oct 29, 2024
Merged

Parse project on target change #1488

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5614925
Parse project on target change
mdesmet Oct 29, 2024
07ec02c
feat: error handling when sync with db
AdiGajbhiye Oct 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/dbt_client/dbtCloudIntegration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1192,6 +1192,8 @@ export class DBTCloudProjectIntegration

async applyDeferConfig(): Promise<void> {}

async applySelectedTarget(): Promise<void> {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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:

  1. Refresh the project configuration
  2. 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.


throwDiagnosticsErrorIfAvailable(): void {
this.throwBridgeErrorIfAvailable();
}
Expand Down
6 changes: 6 additions & 0 deletions src/dbt_client/dbtCoreIntegration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1199,6 +1199,12 @@ export class DBTCoreProjectIntegration
(python) =>
python!`project.set_defer_config(${deferToProduction}, ${manifestPath}, ${favorState})`,
);
await this.refreshProjectConfig();
await this.rebuildManifest();
}
Comment on lines +1202 to +1204
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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();
}
Comment on lines +1206 to 1209
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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;
}
}


Expand Down
1 change: 1 addition & 0 deletions src/dbt_client/dbtIntegration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,6 +390,7 @@ export interface DBTProjectIntegration extends Disposable {
args: HealthcheckArgs,
): Promise<ProjectHealthcheck>;
applyDeferConfig(): Promise<void>;
applySelectedTarget(): Promise<void>;
getAllDiagnostic(): Diagnostic[];
throwDiagnosticsErrorIfAvailable(): void;
getPythonBridgeStatus(): boolean;
Expand Down
13 changes: 12 additions & 1 deletion src/manifest/dbtProject.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
EventEmitter,
FileSystemWatcher,
languages,
ProgressLocation,
Range,
RelativePattern,
Uri,
Expand Down Expand Up @@ -219,7 +220,17 @@ export class DBTProject implements Disposable {
}

async setSelectedTarget(targetName: string) {
await this.dbtProjectIntegration.setSelectedTarget(targetName);
await window.withProgress(
{
location: ProgressLocation.Notification,
title: "Changing target...",
cancellable: false,
},
async () => {
await this.dbtProjectIntegration.setSelectedTarget(targetName);
await this.dbtProjectIntegration.applySelectedTarget();
},
);
}

getDBTProjectFilePath() {
Expand Down
Loading
Loading