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

DBinterface.jl Support #271

Closed
wants to merge 7 commits into from
Closed

Conversation

chris-b1
Copy link
Contributor

potentially closes #206, cc @TheCedarPrince

@iamed2
Copy link
Collaborator

iamed2 commented Mar 28, 2023

I noticed you added a prepare dispatch but didn't test it, is there a reason for that? Could you add a small test? Otherwise this looks good.

@chris-b1
Copy link
Contributor Author

I noticed you added a prepare dispatch but didn't test it, is there a reason for that? Could you add a small test? Otherwise this looks good.

Good catch, just added a test.

test/runtests.jl Outdated Show resolved Hide resolved
Co-authored-by: Eric Davies <[email protected]>
test/runtests.jl Outdated Show resolved Hide resolved
Co-authored-by: Eric Davies <[email protected]>
@@ -0,0 +1,9 @@
DBInterface.connect(::Type{Connection}, args...; kws...) = Connection(args...; kws...)

DBInterface.prepare(conn::Connection, args...; kws...) = prepare(conn, args...; kws...)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why this function needs to have separate, identically named bindings with identical method signatures in DBInterface and LibPQ? If a dependency on DBInterface is added anyway, it seems like the current definition of prepare could just extend then reexport the binding from DBInterface. Same for execute.

Copy link
Collaborator

Choose a reason for hiding this comment

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

LibPQ.jl is not currently fully compliant with the interface. The solution presented here avoids bringing it into compliance in favour of providing partial compliance. The future solution to bring it into compliance is either to augment LibPQ.jl's interface to bring it to compliance, or to wrap that interface with a compliant one, and this strategy allows both options.

An example of partial compliance: prepare does not have a do-block form. In LibPQ.jl, I think it would make the most sense if that form actually used the function to operate on the statement and then deallocate the statement at the end, rather than the DBInterface method which says the function should be f()::DBInterface.Connection (I don't see why that would be particularly useful). Having separate dispatches for these two functions allows both to be implemented in the future.

@TheCedarPrince
Copy link
Collaborator

Hey is there anything I can do to help with finishing this PR and getting it merged @iamed2 and @ararslan ? Seems like it is almost there. Thanks!

@iamed2
Copy link
Collaborator

iamed2 commented Jun 27, 2023

Can you rebase on the latest master to trigger CI again? I would expect it to pass with the exception of the docs jobs which are currently broken (I probably need to do a manifest update). Then I'm happy to merge and release.

@TheCedarPrince
Copy link
Collaborator

Absolutely; although I am not the original creator of this PR so if @chris-b1 doesn't respond before, I'll open a separate PR incorporating their changes so we can get this merged in. Thanks @iamed2 !

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.

DBInterface.jl support
4 participants