Skip to content
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

Improvements to parsing into Utc, converting from other timezones to Utc #263

Closed
mqudsi opened this issue Jul 13, 2018 · 4 comments
Closed

Comments

@mqudsi
Copy link
Contributor

mqudsi commented Jul 13, 2018

Thanks for making dealing with dates and times in rust easier and for providing what has become the defacto standard for date-based operations.

I do believe there is room for improvement however when it comes to dealing with DateTime<Utc> instances; unless I'm mistaken and there's actually an easier way, this is what my code currently looks like to parse a timestamp from a string into a DateTime<Utc> instance:

        let created: DateTime<Utc> = DateTime::from_utc(
            DateTime::<FixedOffset>::parse_from_rfc3339("1996-12-19T16:39:57-08:00")
                .unwrap()
                .naive_utc(),
            Utc,
        );

Aside from the fact that it isn't very pretty or terse, I think it's also extremely undiscoverable. In particular, the usage of DateTime::from_utc to convert to a DateTime<Utc> object is unexpected (I understand why it works).

It would be extremely helpful if DateTime<FixedOffset> had a to_utc() helper method or some other means of converting more intuitively between DateTime flavors.

Is there a fundamental reason why there are no std::convert impls to facilitate conversion between these types? Since in the code above there is only the first .unwrap() and all other operations are not expected to ever produce an error, I imagine it would be possible to use std::convert in its current form (vs waiting for std::convert::TryFrom to finally stabilize).

If std::convert::From<chrono::DateTime<chrono::FixedOffset>> were implemented for chrono::DateTime<chrono::Utc> (or std::convert::Into<chrono::DateTime<chrono::Utc>> implemented for chrono::DateTime<chrono::FixedOffset> if one type isn't aware of the other) the code above could be simplified considerably:

        let created: DateTime<Utc> = DateTime::<FixedOffset>::parse_from_rfc3339("1996-12-19T16:39:57-08:00").unwrap().into();

(Although I would still suggest that parse_from_rfcxxxx be implemented for DateTime<Utc> in all cases.)

@dpc
Copy link

dpc commented Aug 10, 2018

I came here, looking for help, found this issue so I'll post my experiences.

I was trying to convert date in Local timezone to a FixedOffset timezone.

The timezone/offset conversions are very confusing and very hard to discover. Some of the traits involved don't even make sense to me: like offset vs timezone.

Also, what is Local if not a FixedOffset timezone of system-configured value? Donno. Maybe I'm missing something.

Some simple conversion functions between dates in different timezone, would be greatly appreciated. :)

@quodlibetor
Copy link
Contributor

Hey, sorry about the delay!

I think that the most fundamental question is why there are no From impls for the various timezones. IMO there is no good reason. #67 actually implemented them, but it was from a time when this library was less-maintained. I tried to rebase so that dbrgn would get the original credit, but ran into semantic conflicts.

I believe that when I last gave it a go the design of the Tz traits was such that it was really awkward to implement conversions pleasantly, but I'm not super confident that that's true.

I agree that From should be implemented between all DateTime<T> and parse_from_xxx should be implemented on Utc. A PR implementing those items would be very welcome, otherwise I'll implement it after RustConf.

@mqudsi
Copy link
Contributor Author

mqudsi commented Aug 12, 2018

No problem. Thanks for replying!

I started working on this in #271 but I'm pretty sure I'm doing something wrong because the approach I took (.with_timezone()) made it much less painful than I'd expected.

Note that it's not possible to implement the conversion in a single impl<Src, Dst> From<DateTime<Src> for DateTime<Dst> where Src: TimeZone, Dst: TimeZone because of the existing rust impl.

mqudsi added a commit to mqudsi/chrono that referenced this issue Sep 6, 2018
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.
mqudsi added a commit to mqudsi/chrono that referenced this issue Apr 8, 2019
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.
mqudsi added a commit to mqudsi/chrono that referenced this issue Aug 31, 2022
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.
mqudsi added a commit to mqudsi/chrono that referenced this issue Aug 31, 2022
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.
djc pushed a commit that referenced this issue Sep 12, 2022
As discussed in #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 #271 for conversion back into `DateTime<Utc>` from
`DateTime<FixedOffset>` as the latter is used under the hood.
Zomtir added a commit to Zomtir/chrono that referenced this issue Mar 22, 2023
- 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>
jtmoon79 added a commit to jtmoon79/chrono that referenced this issue May 28, 2023
More tests for combinations of `DateTime::into`.

Follow-up to Pull Request chronotope#271.
Issue chronotope#263
@pitdicker
Copy link
Collaborator

Fixed in #271.

Since then you can write:

let created: DateTime<Utc> = DateTime::parse_from_rfc3339("1996-12-19T16:39:57-08:00").into();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants