-
Notifications
You must be signed in to change notification settings - Fork 56
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
Simplify date handling #119
Conversation
A major backwards-compatibility break, unfortunately. The binary parser’s approach of creating a local timestamp then adjusting it by its timezone offset to get the same components in UTC just doesn’t work, producing the wrong results around daylight saving transitions, for example. But even when it’s fixed to work as well as the text parser does, some times are still completely unrepresentable because they don’t exist in local time (daylight saving transitions are, again, an example of this). The more reliable way is: - `timestamptz` represents an instant in time - `timestamp` preserves its components in UTC - `date` remains a string
334cb91
to
4054131
Compare
Related: #50, #81, brianc/node-postgres#2154, brianc/node-postgres#1200 (comment), brianc/node-postgres#993, brianc/node-postgres#783 many more. pg’s Any ideas on making this less hazardous for people upgrading? Or maybe this change just can’t be made at all? Opt-in with a warning for a whole major version, maybe? |
I'm definitely for getting things correct even if there are upgrade hazards. I'm not a big believer in warnings since they tend to go unnoticed, especially if they're not on startup or install. I think a major version with multiple significant type changes (this, int8 as BigInt, and any others) is the clearest approach. |
to match the new [email protected] behavior, compared to 1.0.4.
Forgot about this, sorry. Just merged #53, trying to collect breaking changes into https://github.com/brianc/node-pg-types/milestone/1. I think the best answer here is to collect these related changes into a single breaking release, provide clear guidance in the release notes, and move forward. Warnings and options go unnoticed by the vast majority of users. Let me know if this PR is ready and I can merge it. |
I think this PR is ready, then. I expect to open a separate one for a binary |
Codecov Report
@@ Coverage Diff @@
## master #119 +/- ##
==========================================
+ Coverage 87.61% 91.70% +4.08%
==========================================
Files 4 4
Lines 210 205 -5
==========================================
+ Hits 184 188 +4
+ Misses 26 17 -9
Continue to review full report at Codecov.
|
A major backwards-compatibility break, unfortunately.
The binary parser’s approach of creating a local timestamp then adjusting it by its timezone offset to get the same components in UTC just doesn’t work, producing the wrong results around daylight saving transitions, for example. But even when it’s fixed to work as well as the text parser does, some times are still completely unrepresentable because they don’t exist in local time (daylight saving transitions are, again, an example of this). The more reliable way is:
timestamptz
represents an instant in timetimestamp
preserves its components in UTCdate
remains a string