date_time
date_time copied to clipboard
boostorg
Fixed a bug for parsing posix timezone: + on the west from UTC, - on the east
Currently mistakenly the signs are used the other way around
Fixes:
- https://svn.boost.org/trac/boost/ticket/4545
- https://svn.boost.org/trac/boost/ticket/3336
PS: It's a follow-up after the https://github.com/boostorg/date_time/pull/28 which I sent to master
The only problem I see with this PR is that it is a breaking change for end-users even if it is the correct calculation. If we make this change we definitely have to alert users of date_time so that they are readily aware of it.
@eldiener yeah, I've been thinking on this as well: every bugfix always is a breaking change of some sort.
So guys, do you need any additional input from me yet? I'm not sure if it's me here blocking it or not.
I am not comfortable taking this patch at this point in the release cycle. I expect a fair amount of pushback from users of Boost.DateTime. I'd prefer to postpone this until after the 1.60 release is made.
I expect a fair amount of pushback from users of Boost.DateTime
How this would happen though? The bug was there for at least 6 (six) years without literally any feedback. What would draw attention after 1.60 is released?
Sadly, ISO 8601 says one thing:
A time zone offset of "+hh:mm" indicates that the date/time uses a local time zone which is "hh" hours and "mm" minutes ahead of UTC. A time zone offset of "-hh:mm" indicates that the date/time uses a local time zone which is "hh" hours and "mm" minutes behind UTC.
while POSIX says the opposite:
offset Indicates the value added to the local time to arrive at Coordinated Universal Time.
I believe a lot of people who use Boost.DateTime are used to the current behavior; and would be unpleasantly surprised if their timestamps changed from +XXXX to -XXXX.
Now that Boost 1.60 has shipped, I would like to resolve this.
Just an FYI; I spoke to Jeff Garland, the author of Boost.DateTime, and his response was "change the documentation, not the code".
Btw, I checked the code once again: even if you fix the documentation the class name
posix_time_zone
still will be misleading, since it does not represent a POSIX timezone.
The same is true for to_posix_string method, which does not serialize an object to a POSIX string.
So, whatever you specify in a documentation - the class and the method names are incorrect. But curious to see how this could be explained in the documentation to not make it even worse than it is now.
Not to say that there is no way to integrate boost with tools and other software that implements the standard properly.
@mclow Perhaps we could make this change conditional on a config macro, disabled by default + a note in release notes. After N releases we could switch the default to the new behavior and have the old behavior conditionally enabled - with another note in release notes.
I'm not comfortable with leaving bugs solely on the purpose of backward compatibility. Bugs have to be fixed (tm).
I agree that this change should be made based on a macro, as specified by mclow. I think the macro should be BOOST_DATE_TIME_SOMETHING... as a date/time config macro. I think we must wait until after the 1.63 is out but that we should work toward this change and can make it in 'develop' now. This merge should not be made unconditionally but forms the basis for the macro-based change. Since I am sort of the macro guy, among Boost developers I am comfortable with making this change and merging it to 'develop'.
@eldiener any chance you keep my contribution so that I at least had left any trace in the project? :-)
There's an ongoing discussion on the boost-maint list regarding this PR. A slightly different solution has been proposed (not involving a config macro).
It looks like this stalled or was not closed off (per the discussion on the mailing list) - was there a resolution?
To my knowledge, the problem is not fixed.
The discussion in the boost-maint list came to a possible solution involving:
- Adding a new function instead of
to_posix_string, that can produce a POSIX or ISO string based on an argument passed by user (e.g. an enum). - Deprecating
to_posix_string. After a few releases it can be removed.
I think the idea was generally agreed upon but noone took the time to implement it.
It is okay to have a breaking change as long as:
- It is noted in the release notes.
- There is a way to enable the old behavior, so it is not breaking.
This is typically used when a library is producing incorrect results, such as this. It looks like there was a wider discussion but nothing much has happened for a few years on this. I am tickling it to see what happens. At the very least it should be rebased on develop so it can run lots of CI.