Skip to content
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

fix: Normalize text protocol handling of numeric and numeric[] types #91

Conversation

sehrope
Copy link
Contributor

@sehrope sehrope commented Aug 16, 2019

NOTE: This does not fix the binary protocol, only the text one.

Normalizes handling of numeric[] types to always return a string array
representation of the value for both the text.

Previously numeric array types were being returned as an array of Number
which could lose precision.
@sehrope
Copy link
Contributor Author

sehrope commented Aug 17, 2019

Closing this out as it was included in #94

@sehrope sehrope closed this Aug 17, 2019
@charmander
Copy link
Contributor

… as one commit labelled “Add linter (standard)”, ee22252. It’s kind of hidden in there. Is it too late to split them? @bendrucker

@bendrucker
Copy link
Collaborator

I haven't published a release to npm. It's been about 24h so I think it's worth it to rewrite the history to split it. Will do now.

@bendrucker bendrucker reopened this Aug 18, 2019
bendrucker pushed a commit that referenced this pull request Aug 18, 2019
Normalizes handling of numeric[] types to always return a string array
representation of the value for both the text.

Previously numeric array types were being returned as an array of Number
which could lose precision.

Closes #91
@bendrucker
Copy link
Collaborator

Did some history rewriting to fix this. Anything else we should be doing around numbers in a breaking release here? It would be nice to give people the options to get BigInts back for this stuff and push towards making this work the way everyone expects (#78) now that it's possible.

@bendrucker
Copy link
Collaborator

Although I guess that really only relates to count/int behavior, not floats. But I'd like to still include a breaking change for #78 (enabled by default if supported) in the same major that releases this PR.

@sehrope
Copy link
Contributor Author

sehrope commented Aug 18, 2019

@bendrucker Thanks for the history cleanup!

I'm a -1 on feature detection changing results. If we're still going to support Node v8 until it's out of LTS then I say release one semver major with the current round of changes and another in Jan 2020 when Node v8 is EOL.

For the record, I'm for the BigInt change itself, just seems brittle to have a node version change the output type of a column. Anything that uses it would have to perform it's own version detection (even the tests for this module).

@yocontra
Copy link

yocontra commented Sep 9, 2019

@sehrope Did you do a follow-up PR? Can't seem to find one.

@sehrope
Copy link
Contributor Author

sehrope commented Sep 9, 2019

@contra Follow up for what?

The text protocol fix for numeric/numeric[] was added in afead5b (it's this PR after some rewriting to fix me accidentally checking in the packed up module). @charmander has a separate branch to redo the binary parsing but it hasn't been submitted as a PR yet.

@yocontra
Copy link

yocontra commented Sep 9, 2019

@sehrope Thanks, exactly what I was looking for.

charmander added a commit to charmander/node-pg-types that referenced this pull request Sep 10, 2019
to match the text parser (and the text array parser, as of brianc#91).
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.

None yet

4 participants