Ok, closing this then. I definitely think the default behaviour needs to be changed to string. The change could be made in the next minor version update, but surely before 1.0. If you are dealing with numeric datatypes you don't normally want to parse to float, since chances are you dealing with arbitrary precision numbers with many decimal places or really large integers.

I apologize if I didn't seem grateful or willing to accept this patch in my last comment. I think what you're saying is completely correct for the long term. I would like to keep this pull request open so I can merge it (and tweak it to not break backwards compatibility in the shorter term.) The work you did is great & definitely will be merged in some form.

I think before 1.0 I'd like to have all these things inital bad decisions worked out and make 1.0 a "clean break" from some of the deprecated/legacy things. It's just tricky to do that too rapidly. According to npmjs.org node-postgres has been downloaded almost 20,000 times in the past month. If we move too quickly on what will surely be a breaking change for some without properly moving the major version of the library I feel that could introduce a lot of pain. Since there's a work around for now by removing the parseFloat with a custom type parser, we can recommend that for now. I'll try to address this issue after I get the pool sorted. Which, by the way, I'm opening a pull request on now...

The problem with maintaining the current approach (versus "breaking" and returning a string) is that the current approach isn't going to work correctly. It will (for some values) still attempt to parse but return a completely incorrect value. See (https://gist.github.com/shtylman/4757910). This is (in my mind) way way worse than someone now having to realize their numeric types are strings. Maybe bump the minor as a result and make it clear in the readme? The reality is that javascript has no Numeric type and attempting to use float is incorrect.

I agree that changing it and bumping the minor should be sufficient ([major].[minor].[patch]) as no one should have this in a package.json:

"pg": "0.x"

They will have:
pg: "0.13.x"

No one should be caught off guard then.

P.S. keeping a Changelog in the repo for each new version would be really helpful.

Y'all have me sold. Will merge this in within a week at the latest.

Regarding the changelog - I have been trying to do all non-trivial changes through a pull request. The github history can be used to generate a "changelog" of sorts along with all the discussion for each change on each pull request. Is this not sufficient?

Is this not sufficient?

It's a lot more convienent I think to be able to read the Changelog directly from a file in the repository in a nice formatted list - especially when you have fallen a couple minor versions back. Maybe I just need to work on my git log skills?

I know what you mean. I think it's nice to read everything in a single file instead of a github page - it's just the amount of effort it takes to keep up to date with a changelog versus just being like "read the github history page". I know I'd go crazy if node versions didn't have a changelog. It's just...it's more work for me and doesn't actually increase the amount of information, just surfaces it better. How about this...when I bump anything other than the breakfix semver digit, I'll document that in a changelog.md file?

Sounds good. I will do this next.

On Saturday, February 23, 2013, freewil wrote:

I agree that changing it and bumping the minor should be sufficient
([major].[minor].[patch]) as no one should have this in a package.json:

"pg": "0.x"

They will have:
pg: "0.13.x"

No one should be caught off guard then.

P.S. keeping a Changelog in the repo for each new version would be really
helpful.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/271#issuecomment-13995638.

I'm thinking maybe we go to v1.0.0 with this change. Thoughts?

I am not against that. Have anything else that you think would be a good
fit for 1.0.0 bump?

On Tue, Feb 26, 2013 at 5:09 PM, Brian C notifications@github.com wrote:

I'm thinking maybe we go to v1.0.0 with this change. Thoughts?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/271#issuecomment-14143007
.

I've been thinking about it, and I don't think a lot is left before 1.0. The API has been pretty stable overall for the past year or so, but I definitely wanted to fix pooling before going to 1.0. That's fixed. Removing the parseFloat would be another good thing to do, since that's backwards incompatible and pretty important. It's used in production by lots of companies, according to npm has had 18k downloads in the past 30 days, well tested unfancy code, etc.

There are a few things I'd like to do which are a bit more major and should be done for a 2.0. Namely, splitting the native & pure javascript into two modules since isaacs was saying "npm post-install" is an antipatern. https://npmjs.org/doc/scripts.html

There are some internal things I'd like to do such as improving the javascript parser but I need to build benchmarks for it first. I can ramble on here for a long time about the things I'd like to work on...this probably ain't the place to do it.

Maybe make a milestone 2.0 on GH issues and add some of these to that? Then
people can slowly start picking them off.

For the native vs purejs you could look at how redis/hiredis do it maybe.

For pooling, do you have to now explicitly give the client back?

On Tue, Feb 26, 2013 at 5:22 PM, Brian C notifications@github.com wrote:

I've been thinking about it, and I don't think a lot is left before 1.0.
The API has been pretty stable overall for the past year or so, but I
definitely wanted to fix pooling before going to 1.0. That's fixed.
Removing the parseFloat would be another good thing to do, since that's
backwards incompatible and pretty important. It's used in production by
lots of companies, according to npm has had 18k downloads in the past 30
days, well tested unfancy code, etc.

There are a few things I'd like to do which are a bit more major and
should be done for a 2.0. Namely, splitting the native & pure javascript
into two modules since isaacs was saying "npm post-install" is an
antipatern. https://npmjs.org/doc/scripts.html

There are some internal things I'd like to do such as improving the
javascript parser but I need to build benchmarks for it first. I can ramble
on here for a long time about the things I'd like to work on...this
probably ain't the place to do it.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/271#issuecomment-14143679
.

I think it lacks a bit of information for users like me here ^^
I found how to hide that annoying warning in the commit history.

pg.defaults.hideDeprecationWarnings = true;

Btw, I have no problem with the actual parseFloat, so will it be possible to use :

require('pg').types.setTypeParser();

yeah, setTypeParser() will be the recommended way to go about converting
the floats into javascript primitives. I'll add a note to the readme about
turning the deprecations off.

On Mon, Mar 11, 2013 at 3:14 PM, nemeria notifications@github.com wrote:

I think it lacks a bit of information for users like me here ^^
I found how to hide that annoying warning in the commit history.

pg.defaults.hideDeprecationWarnings = true;

Btw, I have no problem with the actual parseFloat, so will it be possible
to use :

require('pg').types.setTypeParser();

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/271#issuecomment-14739771
.

I would suggest adding a note about why you want to avoid using parseFloat and some possible numeric libs the user can use. It is important that this issue is very clear lest people attempt to do maths and fail :)

closing this - let's move any further discussion to #301

I think part of the problem here is that the library doesn't include data type information in the results themselves, so a client that's willing to distinguish between a number and a string can only do it by looking at the JS datatype resulting for it. See #209