-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
allow both connectionString and additional options #1363
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you be able to include some tests for this? I can't accept a PR without tests.
.gitignore
Outdated
@@ -4,3 +4,4 @@ node_modules/ | |||
.lock-wscript | |||
build/ | |||
*~ | |||
package-lock.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove this
package.json
Outdated
"packet-reader": "0.3.1", | ||
"pg-connection-string": "0.1.3", | ||
"pg-native": "^1.10.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should not be a dependency
package.json
Outdated
@@ -19,8 +19,10 @@ | |||
"main": "./lib", | |||
"dependencies": { | |||
"buffer-writer": "1.0.1", | |||
"object-assign": "^4.1.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this needs to be exactly 4.1.1
without the ^
Thanks @caub! Put a couple comments up there. |
ok, I think npm5 automatically adds pg-native, since it's required in the lib/ source. And it generates that package-lock thing too I wanted to test with |
ohhhh I bet it added it during the test run. I'll need to add some kinda parameter to the test to make it not save when doing an |
lib/connection-parameters.js
Outdated
@@ -11,6 +11,9 @@ var dns = require('dns'); | |||
|
|||
var defaults = require('./defaults'); | |||
|
|||
var parse = require('pg-connection-string').parse; //parses a connection string | |||
var objectAssign = require('object-assign'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Object.assign
can be used now that pg targets Node 4.
@charmander done, that's great to be node4+ now |
lib/connection-parameters.js
Outdated
@@ -43,12 +42,12 @@ var useSsl = function () { | |||
|
|||
var ConnectionParameters = function (config) { | |||
// if a string is passed, it is a raw connection string so we parse it into a config | |||
config = typeof config === 'string' ? parse(config) : (config || {}) | |||
config = typeof config === 'string' ? parse(config) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be a bit too compact. Maybe:
config = typeof config === 'string' ? parse(config) : Object.assign({}, config)
if (config.connectionString) {
Object.assign(config, parse(config.connectionString))
}
?
lib/connection-parameters.js
Outdated
config = parse(config.connectionString) | ||
} | ||
var config = typeof cfg.connectionString === 'string' ? Object.assign({}, cfg, parse(cfg.connectionString)) : cfg | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've used intermediate vars, or I can mutate if you really prefer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’d still suggest the same snippet as earlier, since it always copies the configuration object.
lib/connection-parameters.js
Outdated
// if the config has a connectionString defined, parse IT into the config we use | ||
// this will override other default values with what is stored in connectionString | ||
if (config.connectionString) { | ||
config = parse(config.connectionString) | ||
if (typeof config.connectionString === 'string') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@charmander ok done, I just check here with typeof, because parse would probably throw else
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Throwing instead of hiding potential errors is generally good, and it already behaved that way. I’d keep this check unmodified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok done
var subject4 = new ConnectionParameters({ | ||
connectionString: 'postgres://test@host/db?ssl=1', | ||
ssl: false, | ||
max: 5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this supposed to work? Connection strings seem to work at the client level, whereas max
is at the pool level.
If it’s not, it should probably be removed to avoid misleading people.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes done, I need to make a similar PR for pg-pool
Thanks so much for this! Pushing out a new minor version w/ the changes now. |
…sts with npm 5 Should save some confusion in future pull requests (brianc#1465, brianc#1436, brianc#1363).
allows
connectionString params will still override other options to stay compatible