-
Notifications
You must be signed in to change notification settings - Fork 533
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
Add std::convert::From conversion between different DateTime offsets #271
Conversation
Adds conversion to/from Utc, Local, and FixedOffset. Originally planned to go through NaiveDateTime, but it seems that is not necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks reasonable, other than adding some doccos and that you need to import some items into scope.
Documentation added to both `impl`s and functions so that they are visible to both users perusing the online documentation and in autocomplete/intellisense engines.
46165c8
to
973c603
Compare
As discussed in chronotope#263. Note that this breaks existing code that did not explicitly specify the offset in calls to `DateTime::parse_from_*`, as they are now ambiguous. Relies on chronotope#271 for conversion back into `DateTime<Utc>` from `DateTime<FixedOffset>` as the latter is used under the hood.
Should be ready for review, @quodlibetor |
@quodlibetor did you get a chance to review this? |
gentle bump |
A gentle bump from me, those conversions would be very useful. |
Sorry this fell of my radar! It looks like the test failures are just a simple Thanks! |
@mqudsi, is there anything left to do on this besides just fixing the test failures? I'm anxious to be able to use this. :P |
Those aren't CI test failures, they're actual build errors. The code has always compiled for me (my only rule is never commit something that doesn't build). Perhaps it has to do with the
Actually, it seems that |
That does make sense, if we can get this merged that would be great! |
Updated w/ the changes requested. Now builds for me locally with |
Thanks, @quodlibetor. btw my |
As discussed in chronotope#263. Note that this breaks existing code that did not explicitly specify the offset in calls to `DateTime::parse_from_*`, as they are now ambiguous. Relies on chronotope#271 for conversion back into `DateTime<Utc>` from `DateTime<FixedOffset>` as the latter is used under the hood.
thanks @mqudsi for this change and thanks @ErichDonGubler for the ping pushing it over the finish line! |
As discussed in chronotope#263. Note that this breaks existing code that did not explicitly specify the offset in calls to `DateTime::parse_from_*`, as they are now ambiguous. Relies on chronotope#271 for conversion back into `DateTime<Utc>` from `DateTime<FixedOffset>` as the latter is used under the hood.
As discussed in chronotope#263. Note that this breaks existing code that did not explicitly specify the offset in calls to `DateTime::parse_from_*`, as they are now ambiguous. Relies on chronotope#271 for conversion back into `DateTime<Utc>` from `DateTime<FixedOffset>` as the latter is used under the hood.
- Creates a global Error enum - Breaks backwards compatiblility mainly because of promoting fallable functions (chronotope#263) - Some tests still fall - Not all doctests are fixed - to_naive_datetime_with_offset function is broken and needs fixing - serde related stuff is not checked properly This is a rebase of chronotope#817 onto the 0.5 main branch. Main differences: - Unify three different error structs - Removed ErrorKind - Adapted a lot of unit tests - Removed some commits from presumably unrelated branches (chronotope#829) or mainlined commits (chronotope#271) Co-authored-by: John-John Tedro <udoprog@tedro.se>
More tests for combinations of `DateTime::into`. Follow-up to Pull Request chronotope#271.
More tests for combinations of `DateTime::into`. Follow-up to Pull Request chronotope#271.
More tests for combinations of `DateTime::into`. Follow-up to Pull Request chronotope#271. Issue chronotope#263
More tests for combinations of `DateTime::into`. Follow-up to Pull Request chronotope#271.
More tests for combinations of `DateTime::into`. Follow-up to Pull Request chronotope#271.
More tests for combinations of `DateTime::into`. Follow-up to Pull Request chronotope#271.
More tests for combinations of `DateTime::into`. Follow-up to Pull Request chronotope#271.
More tests for combinations of `DateTime::into`. Follow-up to Pull Request chronotope#271.
More tests for combinations of `DateTime::into`. Follow-up to Pull Request chronotope#271.
More tests for combinations of `DateTime::into`. Follow-up to Pull Request chronotope#271.
Adds conversion to/from Utc, Local, and FixedOffset. Originally planned
to go through NaiveDateTime, but it seems that is not necessary?
Ref #263