Skip to content

Add test for using intervals as parameter in prepared statements #719

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

Merged
merged 1 commit into from
Feb 22, 2015

Conversation

marekventur
Copy link
Contributor

Currently node-postgres returns intervals in the form of {days: 12, hours: 2}. Passing the same object back into a prepared statement causes strange behaviour and is normally not correct.

This just adds a test that will fail. The actual fix is here: brianc/node-pg-types#16

@brianc
Copy link
Owner

brianc commented Feb 21, 2015

Hey @marekventur -

I'm really sorry about taking so long on this. 😬 I've started a new job and was kinda going full blast on it and not saving anytime for OSS for a bit. Back now! 💃

I'll go ahead & merge that change into node-pg-types. Once that's integrated, want to bump your version you use of node-pg-types on your branch so the tests pass? I actually think it might be best to relax the versioning requirement for node-pg-types to just require a particular major version, letting the minor and patch versions increment naturally. When I'm using other people's modules I generally pin to the exact semver because I've been hit by breaking changes in minor and patch versions of other peoples modules, but since we control both node-postgres and node-pg-types we know we wont put breaking changes into a patch or minor version over there! How does that sound?

@brianc
Copy link
Owner

brianc commented Feb 21, 2015

Actually since I kept you waiting so long I'll just merge this & do the rest myself! Sorry again for that.

brianc added a commit that referenced this pull request Feb 22, 2015
Add test for using intervals as parameter in prepared statements
@brianc brianc merged commit 7613d9c into brianc:master Feb 22, 2015
@marekventur
Copy link
Contributor Author

Hi!

I hope you had a good start at your job! Sorry for keeping you waiting too, I was on holiday without good internet connection. Your idea looks good to me, mildly relaxing the version requirements for bits you control sounds sensible too. Since you've already done the merge there's nothing left for me to do, thanks :) Now I can update our app to the newest version of node-postgres and remove some dirty work-around code that's no longer necessary.

Marek

@brianc
Copy link
Owner

brianc commented Feb 25, 2015

Awesome! Please lemme know if it doesn't work or you hit any snags! :)

On Wed, Feb 25, 2015 at 6:09 AM, marekventur notifications@github.com
wrote:

Hi!

I hope you had a good start at your job! Sorry for keeping you waiting
too, I was on holiday without good internet connection. Your idea looks
good to me, mildly relaxing the version requirements for bits you control
sounds sensible too. Since you've already done the merge there's nothing
left for me to do, thanks :) Now I can update our app to the newest version
of node-postgres and remove some dirty work-around code that's no longer
necessary.

Marek


Reply to this email directly or view it on GitHub
#719 (comment).

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.

2 participants