Client.prototype.connect with callback

[booo]

What do you think about adding an optional callback function to the Client.prototype.connect function?

pg = require "pg"

client = new pg.Client "pg://postgres:testing@localhost/orm"

client.connect (error, client) ->
  if error then console.log error
  else
    console.log "connected (callback)"
    client.end()

client.on "connect", ->
  #never reached
  console.log "connected..."
  client.end()

client.on "error", (error) ->
  #only for errors afther connect event
  console.log error

I prepared a patch. Maybe you can take a look and tell me where the best place for test is.

Regards
Philipp

[brianc]

whoops - i was logged in with my work account....anyways, reposting...

I like this! Happy to get some pull request action too. You're right on the money about suppressing the error event from being emitted for connection errors if the callback is supplied to the connect method. I think you might want to test this here:

https://github.com/brianc/node-postgres/blob/master/test/integration/client/error-handling-tests.js#L93

The easiest way I could think of failing the connection was to just connect to an invalid host...this will have the stream connect method fail. Also there should probably be a test for connecting to a valid postgres server but with invalid credentials. Reason being the stream connecting to the socket and the intial 'handshake' between the client and server are two sort of separate parts of the connection process. In both failure cases, the callback should be called with an error. In the case of the stream failing it will likely be a node Exception and if the handshake fails, it will likely be a postgres error hash. Most of this already is in the client here: https://github.com/brianc/node-postgres/blob/master/lib/client.js#L31 just need to suppress the error event if a callback is supplied.

Hope that makes sense. Anyways - looking forward to the patch.

Do you want me to merge that other pull request or do you just want to bundle that up w/ the connect callback functionality into a single patch?

[brianc]

alrighty - merged this in. Thank you so much!