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

added NextResultSet implementation #747

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

kmpm
Copy link

@kmpm kmpm commented Jun 21, 2021

Trying to revive the solution for issue #380

Shamelessly used the stuff that @basvanbeek created in pr #453 but added some testing with postgres.
This would also work with mssql but I didn't want to mess with the test environment.

@kmpm
Copy link
Author

kmpm commented Sep 6, 2022

@jmoiron, what is the preferred way of getting something like this included?

@BenKnigge
Copy link

This project appears to have been abandoned I've merged this into my own fork here https://github.com/BenKnigge/sqly
If anyone else would be interested in going through this pr black log and merging them into my fork the help would be welcomed.

@tazimmerman
Copy link

This project appears to have been abandoned I've merged this into my own fork here https://github.com/BenKnigge/sqly If anyone else would be interested in going through this pr black log and merging them into my fork the help would be welcomed.

@BenKnigge Should we use your fork or https://github.com/go-sqlx/sqlx? I recall seeing your name link to that fork as well in another issue or PR comment.

@dlsniper dlsniper added could merge The PR look safe enough to merge needs testing The PR needs more testing before being accepted labels Feb 1, 2024
@kmpm
Copy link
Author

kmpm commented Feb 1, 2024

@dlsniper, what kind of testing do you have in mind?
I have used it with sqlserver alot, but I don't know how much work it would be to add it to the test environment.

@dlsniper
Copy link
Collaborator

dlsniper commented Feb 1, 2024

Hi @kmpm I've tagged this as needs testing because it's something that would benefit having the automated CI tests in place before merging. I plan to integrate that in the extremely near future and then I think it will be good to go. I'll take a closer look at it in the next couple of weeks or so, there are plenty PRs to get to and this is more complex than some of the other ones.

Thank you for being so patient about it and for your contribution.

@kmpm
Copy link
Author

kmpm commented Feb 1, 2024

The most complex thing with this PR is actually the test. The funtional part of the code is actually quite straight forward .

@kmpm
Copy link
Author

kmpm commented May 24, 2024

Hi @DSniper, what kind of tests are you missing.
Preferably there should be some tests for mssql but that is not one of the database engines in the default test environment for this project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
could merge The PR look safe enough to merge needs testing The PR needs more testing before being accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants