Proposed solution to issue #50 #53
Conversation
…alue is returned unchanged.
|
This looks good to me & looks like a good call! Ultimately @bendrucker is the final say on this repo, but if y'all merge this please open a PR on node-postgres to bump the major-version of node-postgres up a notch as well. That'll serve as a perm reminder I need to get my ducks in a row for what all breaking changes I want in the next version of node-postgres. This is a pretty world-shattering breaking change if you're not expecting it and rely on existing behavior, so we'll be careful to advertise it and provide an example in the release notes on how you could revert to old behavior if you wanted. We're probably going to drop support for versions of node < 4.0 too so it'll be good to combo this with that as a single new major version. |
|
also: apologies for being like 200 years late with my feedback, been kinda crushed w/ life responsibilities these days. |
|
Same goes for me. I'm transitioning into dev ops work and can justify budgeting more open source time for node-postgres. I'd like to keep this PR open until that breaking node-postgres release is ready. |
|
okay Rad - just settling down one final discussion & should know soon! Thanks so much for everything you've done here @bendrucker! ❤️ |
|
@brianc that's ok, life happens! |
There was a problem hiding this comment.
Hello!
A bit of an outsider, but decided to check out the code. Made the comments I considered valuable. Since dates are to be left as is, remove them all together from the textParsers.
Removing the parseDateArray (using parseStringArray in it's place) and parseDate functions, since they are no longer needed.
| var parseDate = function(value) { | ||
| return value; | ||
| } | ||
|
|
| @@ -162,9 +179,9 @@ var init = function(register) { | |||
| register(1015, parseStringArray); //varchar | |||
| register(1008, parseStringArray); | |||
| register(1009, parseStringArray); | |||
| register(1115, parseDateArray); // timestamp without time zone[] | |||
| register(1115, parseTimestampArray); // timestamp without time zone[] | |||
| register(1182, parseDateArray); // _date | |||
There was a problem hiding this comment.
change to parseStringArray
| @@ -145,8 +162,8 @@ var init = function(register) { | |||
| register(701, parseFloat); // float8/double | |||
| register(16, parseBool); | |||
| register(1082, parseDate); // date | |||
|
@chamini2 that's a neat way to save a function call 👍 |
|
Hi, // query: "SELECT current_date as TODAY" Is this bug already solved? Should I query in a different way ? Am I doing something wrong ? Thanks in advance |
|
Are maintainers still interested in merging this fix? |
@glococo, this is becuase the machine you are running this in has a time zone of |
|
Maybe 2020 is the year this could be fixed? 😉 |
|
Or the year to consider porting your codebases to a language with a better type system 😄 Jokes aside, if the maintainer needs any help with this, I'd be willing to lend a hand. |
|
There are two type parsing behaviors with sharp edges:
With Node 10 in maintenance LTS now seems like a good time to do the latter. No reason to hold this up other than forgetting about it. If you can fix the conflict, happy to merge and release both of the above changes in a new major. |
Timestamps are now parsed differently from YYYY-MM-DD date strings. Value is returned unchanged.
Please see #50