Would you like to pull my changes? #20
Conversation
Conflicts: lib/query.js
Conflicts: lib/query.js
|
I've been watching your repo for a while. I'm excited about the work you've done; however, I cannot merge this in without automated testing to prove it works & support future development. I would like to merge this, but please write tests covering the code. Once I get the libpq bindings in place I'll probably have time to help you out with writing tests. It would be good to support the binary protocol in both the libpq bindings as well as the pure javascript client. Alternatively you can look at a way to run the existing integration tests but have them all use the binary protocol instead of text protocol via a command line switch. I've done something similar with the recent addition of libpq bindings. I did write a few new tests for the libpq bindings, but most of the testing was performed by having the existing integration tests retrofitted to require the native libpq bindings rather than the pure javascript driver. This way I can test both forms of the library adhere to the same interface and emit the same events. tl; dr - I really like this, but I cannot accept a patch without tests as it violates one of the foundational concepts of node-postgres. |
|
Hi, Thanks |
|
Thanks @AlexanderS! Could you sync up your branch to the current master branch of my repository by merging them together & then send me a pull request with the merged code? |
Conflicts: lib/query.js test/unit/client/typed-query-results-tests.js
|
Merged my code with your master. |
|
When I check out your branch & run the tests it fails. Are all of the tests passing for you? Example: https://gist.github.com/1074819 |
|
Fixed it. That was just a rename of the getStringTypeParser function... |
|
Do all of the tests pass for?
When I run that command I get a failure on one of the very first integration tests. |
Conflicts: lib/query.js lib/types.js test/unit/client/query-tests.js test/unit/client/typed-query-results-tests.js
some textParsers requires the input value to be a string, so convert it before calling the textParsers the same problem exists in test/integration/connection/query-test so that there also need to be a String call
native bindings need to get the textParsers with the new syntax
queries like {text: ""} did not get recognized correctly before and
get converted to {text: {text: ""}}
if connection is created with config.binary = true, all queries get executed with binary result unless explicit disabled with binary = false
WARNING: bigint support is not correctly working for really big values. If the value of a integer gets big the number gets fuzzy in javascript. This is not a limitation of this library. If you want to handle bigint with the exact value, get it as string and do not calculate things with it!
instead of the connection string use the config dict in all tests to be able to specify things like binary mode
integration tests could now be started in binary mode some tests are executed in text mode anyway, they are currently not compatible with binary mode or prepared statements at all (f.e. multiple statements in one query)
|
Changed a lot of stuff. All tests pass now. I also added a possibility to run the integration tests in binary mode. Hope you like it. :) |
|
awesome work. thank you! |
Hi,
I do not know if my code is good enough for you. Until now I had no time to implement tests or something like that. The binary and text protocol can be changed with the binary attribute in the query.
Let me know, if you want me to add or change something before merge.
Thanks,
Alex