Skip to content

Test that Exec and Query are only called with certain types#351

Open
cbandy wants to merge 1 commit into
lib:masterfrom
cbandy:driver-contract
Open

Test that Exec and Query are only called with certain types#351
cbandy wants to merge 1 commit into
lib:masterfrom
cbandy:driver-contract

Conversation

@cbandy

@cbandy cbandy commented Mar 15, 2015

Copy link
Copy Markdown
Contributor

Because driver.Value is implemented as an interface{}, its value can actually be any type. It is the responsibility of the sql and driver packages to emit only a few types for these values.

This test ensures those packages are fulfilling their contract in cases we care about.

@johto

johto commented Mar 16, 2015

Copy link
Copy Markdown
Contributor

What was the conclusion with the float32 handling in encode()? Is it dead code?

@cbandy

cbandy commented Mar 16, 2015

Copy link
Copy Markdown
Contributor Author

Yes, it's dead.

@cbandy

cbandy commented Apr 2, 2015

Copy link
Copy Markdown
Contributor Author

@johto would you like me to include a commit that removes the dead code?

@johto

johto commented Apr 4, 2015

Copy link
Copy Markdown
Contributor

@cbandy: Please do.

@cbandy

cbandy commented Apr 4, 2015

Copy link
Copy Markdown
Contributor Author

I've added a commit that removes code for encoding float32 values. The tests for #196, #197, and #198 remain and pass.

@johto

johto commented Apr 8, 2015

Copy link
Copy Markdown
Contributor

I've added a commit that removes code for encoding float32 values. The tests for #196, #197, and #198 remain and pass.

Thanks! I'll try and have a look at this later today.

@cbandy

cbandy commented Apr 22, 2015

Copy link
Copy Markdown
Contributor Author

@johto I think this (or a test like this) is still valuable to include in our suite. What would you like me to do with this PR?

@johto

johto commented Apr 22, 2015

Copy link
Copy Markdown
Contributor

@johto I think this (or a test like this) is still valuable to include in our suite. What would you like me to do with this PR?

I agree, but I haven't had a chance to review it yet. Sorry about that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants