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

SQLite client #1764

Merged
merged 12 commits into from Nov 17, 2023
Merged

SQLite client #1764

merged 12 commits into from Nov 17, 2023

Conversation

not-night-but
Copy link
Contributor

@not-night-but not-night-but commented Oct 27, 2023

Close #1739

This PR refactors the sqlite client to be a proper class 😀
It also simplifies the query execution methods.

@not-night-but not-night-but marked this pull request as ready for review October 31, 2023 09:05
apps/studio/src/lib/db/clients/sqlite.ts Outdated Show resolved Hide resolved
apps/studio/src/lib/db/clients/sqlite.ts Outdated Show resolved Hide resolved
apps/studio/src/lib/db/clients/sqlite.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@rathboma rathboma left a comment

Choose a reason for hiding this comment

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

Awesome work Day! Great stuff. Only minor changes needed:

Let's not use as and just have two methods. Also - remove the stuff you told yourself to remove :-)

You have a bunch of notes about broken or incomplete things. Can they be dealt with or better notes left?

apps/studio/src/lib/db/clients/sqlite.ts Outdated Show resolved Hide resolved
apps/studio/src/lib/db/clients/sqlite.ts Outdated Show resolved Hide resolved
apps/studio/src/lib/db/clients/sqlite.ts Outdated Show resolved Hide resolved
apps/studio/src/lib/db/clients/sqlite.ts Outdated Show resolved Hide resolved
apps/studio/src/lib/db/clients/sqlite.ts Outdated Show resolved Hide resolved
apps/studio/src/lib/db/clients/sqlite.ts Outdated Show resolved Hide resolved
apps/studio/src/lib/db/clients/sqlite.ts Outdated Show resolved Hide resolved
apps/studio/src/lib/db/clients/sqlite.ts Outdated Show resolved Hide resolved
}

createDatabaseSQL(): string {
throw new Error("Method not implemented.");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we need a NotSupportedError that can be handled in a standard way in the UI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that's a good idea. Would be super useful for the readonly clients as well

apps/studio/src/lib/db/clients/sqlite/SqliteCursor.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@rathboma rathboma left a comment

Choose a reason for hiding this comment

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

Looks good. Let's merge it.

@not-night-but not-night-but merged commit df1a2a3 into master Nov 17, 2023
4 checks passed
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.

FEAT: Migrate SQLite to a class-based database driver
2 participants