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

Connections: Create SQLite connection #6380

Merged
merged 8 commits into from
Feb 20, 2025
Merged

Connections: Create SQLite connection #6380

merged 8 commits into from
Feb 20, 2025

Conversation

dfalbel
Copy link
Contributor

@dfalbel dfalbel commented Feb 17, 2025

Addresses #6185
Additionaly, add support for checking and installing package dependencies when they are not installed.

Release Notes

New Features

  • N/A

Bug Fixes

  • N/A

QA Notes

Try creating a new SQLite connection with both R and Python. Make sure the generated code executes correctly.
Possibly try running in an R environment where packages are not available, so you can test the installation steps.

@:connections

Copy link

github-actions bot commented Feb 17, 2025

E2E Tests 🚀
This PR will run tests tagged with: @:critical @:connections

readme  valid tags

@dfalbel dfalbel marked this pull request as ready for review February 18, 2025 13:08
@dfalbel dfalbel requested a review from jmcphers February 18, 2025 13:52
}

for (const pkg of this.packages) {
const installed = await session.callMethod?.('is_installed', pkg);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs some exception handling; if callMethod throws I think the exception should be logged (and we should probably presume the dependencies to be installed since otherwise the user will be blocked as I understand it?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In theory this is handled on the call site of checkDependencies and installDependencies:

try {
if (!props.selectedDriver.connect) {
throw new Error(
localize('positron.newConnectionModalDialog.createConnection.connectNotImplemented', "Connect method not implemented")
);
}
if (props.selectedDriver.checkDependencies && props.selectedDriver.installDependencies) {
const dependenciesInstalled = await props.selectedDriver.checkDependencies();
if (!dependenciesInstalled) {
await props.selectedDriver.installDependencies();
}
}
await props.selectedDriver.connect(code);
} catch (err) {
services.notificationService.error(err);
}

But indeed, they wouldn't be able to run connect() if checking for the dependencies fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With 2bfb463 we now presume that upon failure to check if dependencies are installed we can continue and try to connect.

}

for (const pkg of this.packages) {
const installed = await session.callMethod?.('is_installed', pkg);
Copy link
Collaborator

Choose a reason for hiding this comment

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

As above, need exception handling here

@dfalbel dfalbel requested a review from jmcphers February 19, 2025 14:20
jmcphers
jmcphers previously approved these changes Feb 20, 2025
Copy link
Collaborator

@jmcphers jmcphers left a comment

Choose a reason for hiding this comment

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

Working for me!

image

Nit: remove space in connections:: connection_view(con)?

@dfalbel dfalbel merged commit a1959ec into main Feb 20, 2025
8 checks passed
@dfalbel dfalbel deleted the new-connections branch February 20, 2025 13:42
@github-actions github-actions bot locked and limited conversation to collaborators Feb 20, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants