Conversation
This reverts commit 8594bc6.
|
Hey @aqeelat - curious what the changes here do exactly? Thanks for doing them! Would you be able to provide a bit more description/context around what they are? Thank you again! |
|
also - the tests are failing 😱 |
|
@brianc the tests were passing locally. I added a commit to specify the timezone. |
| if (!Number.isInteger(oid)) { | ||
| throw new TypeError('oid must be an integer: ' + oid) |
There was a problem hiding this comment.
This change breaks the check specified in:
https://github.com/brianc/node-postgres/blob/50c06f9bc6ff2ca1e8d7b7268b9af54ce49d72c1/packages/pg/test/unit/client/throw-in-type-parser-tests.js#L11-L13
| const result = new Date((rawValue / 1000) + 946684800000) | ||
|
|
||
| if (!isUTC) { | ||
| result.setTime(result.getTime() + result.getTimezoneOffset() * 60000) | ||
| } | ||
|
|
||
| // add microseconds to the date | ||
| result.usec = rawValue % 1000 | ||
| result.getMicroSeconds = function () { | ||
| return this.usec | ||
| } | ||
| result.setMicroSeconds = function (value) { | ||
| this.usec = value | ||
| } | ||
| result.getUTCMicroSeconds = function () { | ||
| return this.usec | ||
| } |
There was a problem hiding this comment.
This change makes the types return a null object for some reason. I don't know why.
| register(700, parseFloat) // float4/real | ||
| register(701, parseFloat) // float8/double | ||
| register(16, parseBool) | ||
| register(1082, parseTimestamp) // date |
There was a problem hiding this comment.
This change makes it so that we return a date object instead of a string
| register(1115, parseTimestampArray) // timestamp without time zone[] | ||
| register(1182, parseStringArray) // date[] | ||
| register(1185, parseTimestampTzArray) // timestamp with time zone[] | ||
| register(1182, parseTimestampArray) // date[] |
There was a problem hiding this comment.
This is related to 1082. We need to use a date parser instead of a string parser.
| const parseTimestamp = function (value) { | ||
| const utc = value.endsWith(' BC') | ||
| ? value.slice(0, -3) + 'Z BC' | ||
| : value + 'Z' | ||
|
|
||
| return parseTimestampTz(utc) | ||
| } |
There was a problem hiding this comment.
this function was very problematic because it appends Z to the end of the timestamp, which breaks all tz related tests. It should be brought back later though.
Because this function was removed, I removed all of the range functions related to it.
| ], | ||
| [ | ||
| '1000-01-01 00:00:00+00 BC', | ||
| dateEquals(-999, 0, 1, 0, 0, 0, 0) |
There was a problem hiding this comment.
This was added to validated converting the datetime string to "Z BC" and is no longer needed.
| value.toISOString(), | ||
| '2010-10-31T00:00:00.000Z' | ||
| ) | ||
| } | ||
| ], | ||
| [ | ||
| '1000-01-01 00:00:00 BC', | ||
| function (t, value) { | ||
| t.equal( | ||
| value.toISOString(), | ||
| '-000999-01-01T00:00:00.000Z' | ||
| value.toUTCString(), | ||
| new Date(2010, 9, 31, 0, 0, 0, 0).toUTCString() |
There was a problem hiding this comment.
This was added to validated converting the datetime string to "Z BC" and is no longer needed. I reverted it to how it was before the change
| ['2010-10-31', function (t, value) { | ||
| const now = new Date(2010, 9, 31) | ||
| dateEquals( | ||
| 2010, | ||
| now.getUTCMonth(), | ||
| now.getUTCDate(), | ||
| now.getUTCHours(), 0, 0, 0)(t, value) | ||
| t.equal(value.getHours(), now.getHours()) | ||
| }] |
There was a problem hiding this comment.
This was added to validated converting the datetime string to "Z BC" and is no longer needed.
| ['(2010-10-31 14:54:13.74,)', tsrangeEquals([[2010, 9, 31, 11, 54, 13, 74], null])], | ||
| ['(2010-10-31 14:54:13.74,infinity)', tsrangeEquals([[2010, 9, 31, 11, 54, 13, 74], null])], | ||
| ['(,2010-10-31 14:54:13.74)', tsrangeEquals([null, [2010, 9, 31, 11, 54, 13, 74]])], | ||
| ['(-infinity,2010-10-31 14:54:13.74)', tsrangeEquals([null, [2010, 9, 31, 11, 54, 13, 74]])], | ||
| ['(2010-10-30 10:54:13.74,2010-10-31 14:54:13.74)', tsrangeEquals([[2010, 9, 30, 7, 54, 13, 74], [2010, 9, 31, 11, 54, 13, 74]])], | ||
| ['("2010-10-31 14:54:13.74",)', tsrangeEquals([[2010, 9, 31, 11, 54, 13, 74], null])], | ||
| ['("2010-10-31 14:54:13.74",infinity)', tsrangeEquals([[2010, 9, 31, 11, 54, 13, 74], null])], | ||
| ['(,"2010-10-31 14:54:13.74")', tsrangeEquals([null, [2010, 9, 31, 11, 54, 13, 74]])], | ||
| ['(-infinity,"2010-10-31 14:54:13.74")', tsrangeEquals([null, [2010, 9, 31, 11, 54, 13, 74]])], | ||
| ['("2010-10-30 10:54:13.74","2010-10-31 14:54:13.74")', tsrangeEquals([[2010, 9, 30, 7, 54, 13, 74], [2010, 9, 31, 11, 54, 13, 74]])] |
There was a problem hiding this comment.
These were failing because of the tz mismatch between my machine and the runner. They should work now.
Also, there's a potential to use Date() objects here but I'm not familiar with the test harness to create fixtures.
|
@brianc I hope my comments explain the changes I made. I also dropped the commit that updates package.json because I didn't want to jump the gun. There might be other changes that we could do such as aligning the node engine version, updating the dependencies, and perhaps adopting typescript. |
|
@brianc were able to look at the comments? |
2 participants
No description provided.