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

Support multiple results sets for go 1.8 #532

Merged
merged 1 commit into from
Nov 3, 2016
Merged

Support multiple results sets for go 1.8 #532

merged 1 commit into from
Nov 3, 2016

Conversation

maddyblue
Copy link
Collaborator

@maddyblue maddyblue commented Nov 2, 2016

@kardianos


This change is Reviewable

@tamird
Copy link
Collaborator

tamird commented Nov 2, 2016

Consider adding 1.8 or tip to .travis with failures allowed. Current code here doesn't compile under 1.8.


Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion.


nextresultset_test.go, line 26 at r1 (raw file):

      }
  }
  if !rows.NextResultSet() {

HasNextResultSet


Comments from Reviewable

@tamird
Copy link
Collaborator

tamird commented Nov 2, 2016

Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion.


nextresultset_test.go, line 26 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

HasNextResultSet

Rather, `HasNextResultSet` should be removed - the methods implemented on `*rows` appear to be incorrect.

Comments from Reviewable

@maddyblue
Copy link
Collaborator Author

Yes, it does compile under 1.8. See "TestMultiple" which only appears in the travis tip builds: https://travis-ci.org/lib/pq/jobs/172536052


Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion.


nextresultset_test.go, line 26 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Rather, HasNextResultSet should be removed - the methods implemented on *rows appear to be incorrect.

No, this is correct. That method is only available to the driver, not on a rows. See https://tip.golang.org/pkg/database/sql/#Rows.NextResultSet and https://docs.google.com/document/d/1F778e7ZSNiSmbju3jsEWzShcb8lIO4kDyfKDNm4PNd8/edit from which this code was taken from.

Comments from Reviewable

@maddyblue
Copy link
Collaborator Author

Renamed the test file because it may be used for the other 1.8 features. Added tests to ensure that extra, unused results don't change future results.

@freeformz
Copy link

Please don't use reviewable.

@tamird
Copy link
Collaborator

tamird commented Nov 3, 2016

Ah, I was under the misapprehension that the database/sql.Rows was an interface, but it is not. LGTM

@maddyblue maddyblue merged commit d8eeeb8 into lib:master Nov 3, 2016
@maddyblue maddyblue deleted the multiple-result-sets branch November 3, 2016 02:43
@itsjamie
Copy link

itsjamie commented Nov 6, 2016

Simply marking this as related to #531

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.

4 participants