-
Notifications
You must be signed in to change notification settings - Fork 14
Spin 3.4 SQLite and PostgreSQL interfaces #81
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
Conversation
Signed-off-by: itowlson <[email protected]>
Because the new code feature-gates several functions and implementations, I added a couple of GH checks to make sure I hadn't messed up the gating. (I had. Of course I had.) These turned up a couple of errors with |
Signed-off-by: itowlson <[email protected]>
Signed-off-by: itowlson <[email protected]>
a4378b2
to
44350d6
Compare
Signed-off-by: itowlson <[email protected]>
cb606c3
to
978a1b3
Compare
Signed-off-by: itowlson <[email protected]>
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.
Code looks good to me. I tested locally and things worked great. Going to ping another maintainer for another set of 👀
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'm not really sure how I feel about keeping around the old interfaces. I understand the reasoning behind it, but I do worry this will be maintenance burden in the future.
/// # } | ||
/// ``` | ||
#[doc(inline)] | ||
pub use super::wit::pg4::Connection; |
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.
We extend the equivalent type in sqlite3: https://github.com/spinframework/spin-rust-sdk/pull/81/files#diff-5d6f50ac9a55cf5e3cd0473391b4195d9aed79c7bd7d2e896c6124b70e8a4416R184-R199
Do we not want the same thing here?
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.
Ther's no concept of default
for Postgres databases (Spin doesn't provide an implementation - it's address-based rather than label-based).
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.
That's true, but the rows
method does seem like it would be useful, no?
} | ||
} | ||
|
||
impl Decode for bool { |
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.
A macro to generate the impls for all of these primitive types might make things easier to read 🤷
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.
How strongly do you feel about doing that as part of this PR? The code here follows pg/pg3
: I like the idea of a macro but I assume we'd prefer to apply it across all versions (or all versions that we retain).
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 don't feel strongly about this. Just a thought.
Fixes #79 and #80
Draft for now because:
Make deps opt-out, and make new code appropriately conditional:Figure out a solution for decodingIt's a horrible solution. But here we areNUMERICRANGE
Testingunits, and example works on my machineExamplesI did one for ranges which are the trap, can do more if suitably motivatedCheck documentationDocument workaround for range query grammar ambiguityNew database APIs for Spin 3.4 spin-docs#119