Open WhyNotHugo opened 2 years ago
Interesting question @WhyNotHugo - it may take a bit of back and forth to settle on a good design for this, however that may be useful to inform a potential redesign/simplification of the TimeZone
and Offset
traits.
From my current understanding of this, I think using an enum
is by far the best option and as an added bonus it avoids the generic parameter. You can probably avoid a case for Utc
and just use FixedOffset
for that which means you only need three cases.
From what I can understand, it would be useful if we made the following changes (I'm not sure whether all are feasible, but they can be investigated):
TimeZone
was object safe, and we had a blanket impl of TimeZone
for Box<dyn TimeZone>
and potentially other smart pointersFixedOffset
everywhere as to avoid the associated typeI'll use an enum
for now, but a tricky aspect for this scenario is that icalendar RRULE
s depend on VTIMEZONE
definitions and vice-versa, so it forces both things to be implemented in the same crate as far as I can tell.
TimeZone was object safe, and we had a blanket impl of TimeZone for Box
and potentially other smart pointers
I think making it object safe would be very useful indeed. At a glance, it seems doable, if the next item can be done too.
we used FixedOffset everywhere as to avoid the associated type
I think this is doable. TimeZone::from_offset
would have to be dropped, and I'm not sure what would be a suitable replacement since I'm not entirely sure what this method is actually used for.
chrono_tz uses its own custom Offset
implementation to carry additional data. Notably, the name and abbreviation of the offset. It would be necessary to consider how this functionality would be implemented if the associated type is always FixedOffset
.
I've done some exploration around possible changes to TimeZone
to make it object safe, and also looking at using FixedOffset
everywhere to avoid the Offset
trait in: https://github.com/chronotope/chrono/pull/830
Yeah, it seems like using FixedOffset
would be a decent option here.
@pitdicker did some change related to this recently get merged?
@WhyNotHugo Sorry, I did not read closely enough, and thought this was solved.
Fox context: I'm parsing icalendar objects, and some fields (like recurrence dates of events) can have multiple date-times in various timezones. This includes Utc, Local (a.k.a. "floating") and timezones encoded into the file which I'll need to decode as a custom
TimeZone
impementation.
For most uses converting all values to DateTime<FixedOffset>
would be the best solution, but not in your case.
I think changing DateTime
to no longer take a generic TimeZone
not is something we can do anytime soon, if ever. It would be quite a fundamental change. And it would come with a bunch of different trade-offs on how to ship/load a timezone database with chrono.
For now it comes down to an enum or trait object. The enum option is not acceptable in your case?
Maybe an enum outside the DateTime
is easier to use?
enum dates {
utc: DateTime<Utc>,
local: DateTime<Local>,
custom: DateTime<MyTimeZone>,
}
You already found https://github.com/chronotope/chrono/issues/432.
I personally don't think trait objects are going to be great for DateTime
's.
It would need a small allocation for every value, and inflate the base type by 2 pointer-sized values.
Had a number of months to think about this, and closing again.
As written in the previous post we have basically two options:
TimeZone
or Offset
traits object-safe.TimeZone
or Offset
traits object-safe.The simplest alternative to our TimeZone
trait is something like:
pub struct DateTime<Off: Offset> { /* ... */ }
pub trait Offset: Clone + Debug {
/// Recreate the offset given a `NaiveDateTime` in UTC.
fn from_utc_datetime() -> Result<Self, Error>;
/// Recreate the offset given a `NaiveDateTime` in local time.
fn from_local_datetime() -> LocalResult<Self>;
/// Convert this offset to a FixedOffset.
fn fix(&self) -> FixedOffset;
}
One of the three fundamental methods can't be made object-safe: a method to lookup an offset when given a local datetime. If the local time is ambiguous we want to return two values of the type Self
(wrapped in a LocalResult
). However an object-safe trait can only ever return a single Self
type, by giving it as an input with &mut self
.
Also using DateTime
with trait objects is just wasteful in terms of size and allocations.
What information would such a type carry? At least
FixedOffset
).String
or fixed-length string slice with an abbrevation of the time zone.String
with the name of the time zone.Local
time, and so should it update when the time zone of Local
changes?Both options are not really realistic. You are better off creating an enum wrapping either the DateTime
s or the TimeZone
s that you want to combine into the same Vec
.
On second thought:
We could potentially let some Offset::update_for_local_datetime
method return a LocalResult<()>
, and you would have to call a second method to get the second variant in case the local time is ambiguous. That would only lead to a performance loss in the rare cases where a local time falls within a time zone transition.
pub struct DateTime<Off: Offset> { /* ... */ }
pub trait Offset: Clone + Debug {
/// Recreate the offset given a `NaiveDateTime` in UTC.
fn update_for_utc_datetime(&mut self) -> Result<(), Error>;
/// Create the offset given a `NaiveDateTime` in local time.
/// Not available when used as a trait object.
fn from_local_datetime() -> LocalResult<Self> where Self: Sized;
/// Recreate the offset given a `NaiveDateTime` in local time.
/// Returns the first result if the local time is ambiguous.
fn update_for_local_datetime_first(&mut self) -> LocalResult<()>;
/// Recreate the offset given a `NaiveDateTime` in local time.
/// Returns the last result if the local time is ambiguous.
fn update_for_local_datetime_last() -> Result<Self>;
/// Convert this offset to a FixedOffset.
fn fix(&self) -> FixedOffset;
}
I need to define a field for one of my types with a collection/list of datetimes which have different timezone types. E.g.: I need something like:
And I need to be able to insert
DateTime<Utc>
,DateTime<Local>
, and otherchrono::offset::TimeZone
implementations.So far, I've tried using
Box<dyn ...>
, but this won't compile since the size is not known at compile time.Is this even possible with the current API? I'm finding the fact that
DateTime
instances are parametrised with theirTimeZone
a bit tricky to work with. I've seen an approach of using anenum
that implementsTimeZone
and each enum variant delegates to is associated type -- but this requires my code to implement an enum variant for any potentialTimeZone
implementation that might be used. This produces an artificial limitation in library code that applications can't use their ownTimeZone
.Fox context: I'm parsing icalendar objects, and some fields (like recurrence dates of events) can have multiple date-times in various timezones. This includes Utc, Local (a.k.a. "floating") and timezones encoded into the file which I'll need to decode as a custom
TimeZone
impementation.Related: https://github.com/fmeringdal/rust-rrule/pull/85