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.

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

syncing with dev/sda/tree api migration #2554

Merged
merged 17 commits into from
Feb 2, 2025

Conversation

tnaum-ms
Copy link
Collaborator

No description provided.

@tnaum-ms tnaum-ms self-assigned this Jan 31, 2025
@tnaum-ms tnaum-ms marked this pull request as ready for review February 2, 2025 19:20
@tnaum-ms tnaum-ms requested a review from a team as a code owner February 2, 2025 19:20
@tnaum-ms tnaum-ms requested review from Copilot and removed request for a team February 2, 2025 19:20
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 77 out of 92 changed files in this pull request and generated 2 comments.

Files not reviewed (15)
  • package.json: Language not supported
  • src/commands/account/copyConnectionString.ts: Evaluated as low risk
  • src/commands/account/registerAccountCommands.ts: Evaluated as low risk
  • src/AzureDBExperiences.ts: Evaluated as low risk
  • src/commands/attachAccount/MongoConnectionStringStep.ts: Evaluated as low risk
  • src/commands/attachEmulator/AttachEmulatorWizardContext.ts: Evaluated as low risk
  • src/commands/attachAccount/attachAccount.ts: Evaluated as low risk
  • src/commands/attachAccount/AttachAccountWizardContext.ts: Evaluated as low risk
  • src/commands/attachAccount/PostgresPasswordStep.ts: Evaluated as low risk
  • src/commands/attachAccount/MongoPasswordStep.ts: Evaluated as low risk
  • src/commands/attachAccount/PostgresUsernameStep.ts: Evaluated as low risk
  • src/commands/attachAccount/PostgresConnectionStringStep.ts: Evaluated as low risk
  • src/commands/attachAccount/DocumentDBConnectionStringStep.ts: Evaluated as low risk
  • src/commands/attachAccount/MongoUsernameStep.ts: Evaluated as low risk
  • src/commands/ViewDatabaseOffer/viewDatabaseOffer.ts: Evaluated as low risk
Comments suppressed due to low confidence (4)

src/commands/attachAccount/PostgresExecuteStep.ts:16

  • Add a check to ensure context.connectionString is defined before using it.
const connectionString = context.connectionString!;

src/commands/attachAccount/DocumentDBExecuteStep.ts:22

  • The error message 'Internal error: connectionString, port, and api must be defined.' should be more specific about which value is missing.
if (api === API.Core || api === API.Table || api === API.Graph || api === API.Cassandra) {

src/commands/attachAccount/MongoExecuteStep.ts:24

  • Ensure parsedCS.username and parsedCS.hosts are not undefined or empty before using them. Add a check to handle these cases.
const label = parsedCS.username + '@' + parsedCS.hosts.join(',');

src/commands/attachEmulator/ExecuteStep.ts:17

  • Add checks for connectionString, port, and experience before using them to make the code more robust.
const connectionString = context.connectionString;

src/commands/attachEmulator/PromptExperienceStep.ts Outdated Show resolved Hide resolved
src/commands/attachEmulator/ExecuteStep.ts Show resolved Hide resolved
Comment on lines 20 to +21
extends vscode.Disposable
implements AzureResourceBranchDataProvider<TreeElementBase>
implements BranchDataProvider<CosmosDBResource, CosmosDBTreeElement>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch! Thanks!

Comment on lines +91 to +94
let clusterInfo: MongoClusterModel = {
...element,
dbExperience: MongoClustersExperience,
} as MongoClusterModel;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

indeed, good tip.

@tnaum-ms
Copy link
Collaborator Author

tnaum-ms commented Feb 2, 2025

Thanks @bk201- for extracting db client calls from Mongo's tree representation. This is a fantastic step towards reducing code duplication! You’ve done an awesome job unifying code execution flows and improving the UX across the board.

Looking ahead, tree items rely on the database client to determine which children to display, and while that might seem like a natural next step for further optimization, I’d personally prefer to keep that direct connection intact :)

I also restored some lost comments.

@tnaum-ms tnaum-ms merged commit fd38df2 into dev/tnaum/tree-api-migration Feb 2, 2025
1 check passed
@tnaum-ms tnaum-ms deleted the dev/sda/tree-api-migration branch February 2, 2025 20:06
@tnaum-ms tnaum-ms restored the dev/sda/tree-api-migration branch February 2, 2025 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants