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

Integration with SQLStrings.jl #216

Merged
merged 5 commits into from
Feb 18, 2022
Merged

Conversation

c42f
Copy link
Contributor

@c42f c42f commented Feb 11, 2021

This integrates the new https://github.com/JuliaComputing/SQLStrings.jl package (discussed in #213) into LibPQ, making it usable with the execute() function.

Currently based on #215 so if reviewing, just ignore the first commit.

This is quite a minimal integration. There's probably other options but this one is most straightforward if you don't mind the dependency.

@c42f c42f changed the title Integration with https://github.com/JuliaComputing/SQLStrings.jl Integration with SQLStrings.jl Feb 11, 2021
@c42f
Copy link
Contributor Author

c42f commented Feb 11, 2021

I'm not sure what's up with the docs build here, SQLStrings is registered at version 0.1. That was quite recent though, maybe it's a stale registry somehow.

@c42f
Copy link
Contributor Author

c42f commented Apr 23, 2021

Bump!

I'd like to get this merged, or alternatively get some feedback on a different more preferred approach.

@c42f
Copy link
Contributor Author

c42f commented Jan 13, 2022

Given the silence here, I guess there's no interest in this integration?

Perhaps I'll resort to Requires.jl to eval the tiny piece of glue code needed to make SQLStrings work out of the box with LibPQ.

@quildtide
Copy link
Contributor

@iamed2

@iamed2
Copy link
Collaborator

iamed2 commented Feb 15, 2022

My apologies for completely missing this. I have started remonitoring this repo for activity.

Happy to merge 7134a4e, but it would be nice to also add a similar method for async_execute here: https://github.com/invenia/LibPQ.jl/blob/master/src/asyncresults.jl#L239

This integrates the SQLStrings package into LibPQ, making it usable with
the execute() function.
@c42f
Copy link
Contributor Author

c42f commented Feb 15, 2022

Great, let's get this merged then. I've rebased to remove the testing infrastructure from this PR and added async_execute along with a simple test of that.

I've tried to appease JuliaFormatter for the files in src but it also wants to reformat large sections of runtests.jl unrelated to this PR, so I'm not sure what to do about that.

@iamed2
Copy link
Collaborator

iamed2 commented Feb 16, 2022

Yeah the JuliaFormatter review integration is broken and I don't like the actual correct suggestions anyway. This looks good to me!

@iamed2
Copy link
Collaborator

iamed2 commented Feb 16, 2022

Ah current Invenia policy is to bump version numbers in merge requests, could you do that (to 1.12.0) now? Then I'll merge and registrate.

@c42f
Copy link
Contributor Author

c42f commented Feb 17, 2022

Project.toml is bumped now

@iamed2 iamed2 merged commit 7b13b3e into JuliaDatabases:master Feb 18, 2022
@c42f c42f deleted the cjf/sqlstrings branch February 20, 2022 09:20
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.

3 participants