-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Adds a method get the underlying sqlite3 pointer #438
Adds a method get the underlying sqlite3 pointer #438
Conversation
GitHub Actions is randomly failing to run The PR looks good to me except.. I think we should require |
I left it as |
I believe it's an antipattern to have a method take It's especially important to take |
@@ -33,6 +34,13 @@ pub struct SqliteConnection { | |||
scratch_row_column_names: Arc<HashMap<UStr, usize>>, | |||
} | |||
|
|||
impl SqliteConnection { | |||
/// Returns the underlying sqlite3* connection handle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like we need to list some do-s and don't-s for this pointer but I'm not sure what those should be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The obvious two that come to mind are not to close the connection or free the pointer. I don't think the Rust docs in the standard library for similar low-level things spell out the do-s and don't-s.
Thanks for the addition. We can always improve docs as we move along. Maybe just a general warning like "preserve the pointer and don't close the connection or bad things will happen". |
Per #430, adds a method
as_raw_handle()
to get the underlyingsqlite3*
from aSqliteConnection
.This PR is cleaner (a single commit). The previous PR also failed CI due to a (hopefully) random fault in docker.