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

All DateTimes must be in UTC! #22

Open
idkq opened this issue May 9, 2021 · 6 comments
Open

All DateTimes must be in UTC! #22

idkq opened this issue May 9, 2021 · 6 comments
Labels
T: Feature Type: New Features

Comments

@idkq
Copy link

idkq commented May 9, 2021

I'm slightly confused as to why "all DateTimes must be in UTC".

Requirement: I want a recurrence every business day of the week (MON-FRI) at 23:59:59 in local time .
Scenario: Let's say user is on UTC-4 and wants a recurrence every day of the week MON-FRI at 23:59:59 in local time. If we convert FRI 23:59:59 to UTC, we now have SAT 03:59:59 which breaks my requirement of every business day of the week. So in the end, when retrieving the instances, I would only have a total of 4 instances since the 5th instance (Friday) is actually on Saturday_UTC, but Friday_Local.

I guess is this a new requirement? To support local recurrence, or is there any other way to deal with that? I am not sure if anyone would care about days of the week in UTC time, since business days/weekends are always relevant in local time.

Obviously I could fake my local time to be UTC, but that would be wrong by definition, and it would only be to retrieve instances.

@idkq idkq added the T: Feature Type: New Features label May 9, 2021
@JonasWanke
Copy link
Owner

The reason for this limitation is that Dart's date/time handling is very limited, and DateTimes can only be in either the local time zone or in UTC. When working with DateTimes in the local time zone, calculations can easily give unexpected results, like the length of a day not being 24 h if summer time changes to winter time on that day or vice versa. Those problems don't occur when working with UTC-based DateTimes, hence I chose those to do all calculations. Note that this package doesn't care about UTC's offset being zero, but only about calculations being correct without caring about peculiarities of individual time zones.

If you're working with DateTimes in the local time zone, I recommend creating a UTC-based DateTime based on the same year/month/day/hour/etc. to work with this package (and not using localDateTime.toUtc(), which preserves the same point in time but can change the values of those fields, making calculations incorrect):

final localDateTime = DateTime.now();
final dateTimeForRrule = DateTime.utc(
  localDateTime.year,
  localDateTime.month,
  localDateTime.day,
  localDateTime.hour,
  localDateTime.minute,
  localDateTime.second,
  localDateTime.millisecond,
  localDateTime.microsecond,
);

@idkq
Copy link
Author

idkq commented May 10, 2021

Thanks - that is what I ended up doing, and it worked fine for now. I have to fake UTC and reverse as well, because the result is not true UTC. My concern was maintenance since, like you said, a developer could try to convert utc/local with .toUtc() or .toLocal() and it wouldn't work.

DateTime _fakeUTC(DateTime nonUtc) {
    return DateTime.utc(nonUtc.year, nonUtc.month, nonUtc.day, nonUtc.hour,
        nonUtc.minute, nonUtc.second, nonUtc.millisecond, nonUtc.microsecond);
}

DateTime _reverseFakeUTC(DateTime fakeUtc) {
    return DateTime(
        fakeUtc.year,
        fakeUtc.month,
        fakeUtc.day,
        fakeUtc.hour,
        fakeUtc.minute,
        fakeUtc.second,
        fakeUtc.millisecond,
        fakeUtc.microsecond);
}

The problem of using UTC is that when retrieving the instances, any timezone specific conditions is lost. If you retrieve an instance post a change in daylight savings, it would not be reflected accordingly.

@plammens
Copy link
Contributor

plammens commented Mar 15, 2023

The current recipe (i.e. copyWith(isUtc: true) and then back again) does seem to work (although due to the complexity of timezones I have the feeling it could fail in some obscure scenario), but it seems to me that it's a cumbersome workaround rather than a solution, and morover as you both have mentioned one could easily fall in the trap of using .toUtc() and .toLocal() (which I've done).

This package is great, and I think this is the most important missing feature, handling local DateTimes directly. There are ways to handle the intricacies of local timezones, such as flutter's DateUtils.addDaysToDate which adds calendar days instead of 24h. The main thing to keep in mind when doing this is that calendar "units" (year, month, week, day) do not correspond to exact duration units: adding day is not the same as adding 24h, adding a week is not the same as adding 7 * 24h. As such they should be handled separately from proper duration units such as hour, minute, etc. (This is why Java has separate Period and Duration classes, for example.). I could look into it if you want.

If you think that it's not worth the trouble, I would at least include the conversion to and from UTC in the package itself, so that users don't have to worry about which is the correct way of converting their local datetimes to a fake UTC datetime and back.

In any case, thanks for this package!

@JonasWanke
Copy link
Owner

I thought about this problem some more and want to offer a different solution: A separate class called, e.g., LocalDateTime or PlainDateTime. This class makes it clear that the stored date and time are independent of any timezone and, hence, timezone details like the change between summer and winter time are not factored into the recurrence calculation.

That class would be inspired by the no longer maintained time_machine package (a port of .NET's Noda Time) and JavaScript's Temporal proposal and its API could look like the following:

@immutable
class LocalDateTime implements Comparable<LocalDateTime> {
  /// Creates an instance based on [dateTime]'s year, day, hour, etc.
  ///
  /// This ignores `dateTime.millisecondsSinceEpoch` and `dateTime.isUtc`.
  LocalDateTime.fromDateTime(DateTime dateTime);

  static LocalDateTime get now => LocalDateTime.fromDateTime(DateTime.now());

  // This class will store the year, month, day, hour, minute, second, and
  // millisecond using helper classes like `LocalDate` and `LocalTime`.

  /// Returns a new [DateTime] with the same year, day, hour, etc. and `isUtc == true`.
  DateTime get dateTimeInUtc;
  /// Returns a new [DateTime] with the same year, day, hour, etc. and `isUtc == false`.
  DateTime get dateTimeInLocalZone;
}

These classes would also have more utilities like LocalDateTime.now, arithmetic operations, as well as parsing and stringification for subsets of ISO 8601 / RFC 3339. Calculations use DateTime internally. They will only support the Gregorian calendar, and related classes like ZonedDateTime and probably localization will be out of scope since I don't have the time to implement or maintain them.

Since these classes are useful outside recurrence rules as well (e.g., in my Flutter package timetable, I could also make good use of LocalDate and enums for the weekday and month), they would live in a separate package.

What do you think about this proposal?

@plammens
Copy link
Contributor

plammens commented Apr 18, 2023

I thought about this problem some more and want to offer a different solution: A separate class called, e.g., LocalDateTime or PlainDateTime. This class makes it clear that the stored date and time are independent of any timezone and, hence, timezone details like the change between summer and winter time are not factored into the recurrence calculation.

I think that could be a good long-term solution. I would call it something different than LocalDateTime, because to me that sounds like a datetime in the local timezone, but instead it's a datetime without any timezone. PlainDateTime, or Python's distinction between "aware" and "naive" date/time objects, sounds better. I personally like how Python's datetime deals with all of this: separate date, time and datetime classes, the naive/aware distinction, etc. The only thing I don't like about it is that naive/aware objects are implemented by the same classes. Your solution (like time_machine) has all of these features and it does separate aware/naive classes.

That class would be inspired by the no longer maintained time_machine package (a port of .NET's Noda Time) and JavaScript's Temporal proposal and its API could look like the following:

I didn't know about the time_machine package. Now that you mention it, it seems to have everything we need. Is it no longer being maintained? I haven't found any warnings in the README or package description page. It sounds like a pain having to re-implement it from scratch. If it's indeed no longer being mainained or doesn't satisfy some of your requirements, perhaps you could make a fork?

@JonasWanke
Copy link
Owner

@plammens Valid point, I think I'll call these classes PlainFoo then. I'll have a more detailed look at Python's version of this topic (and also look at Kotlin/Java), but also prefer separate classes for date/time with or without time zone information (“aware”). (I'll probably also add an Instant class that's basically DateTime.utc.)

Actually, both rrule and timetable once used time_machine, but, due to bugs and lacking maintenance of that package, users requested a change away from time_machine: JonasWanke/timetable#33, #16, and #9. Both packages now use Dart's native DateTime and have asserts that the objects are valid. In particular, time_machine uses assets for time zone information but Dart doesn't offer a way to ship assets with code (unlike Flutter, where that's pretty easy). Hence, these asset folders also have to be present at the correct place depending on how the code is distributed (see Dana-Ferguson/time_machine#25, Dana-Ferguson/time_machine#53, and also Dana-Ferguson/time_machine#55). That's also why I do have a fork of time_machine: https://github.com/JonasWanke/time_machine. But time_machine contains a lot of code for timezone handling using data from the Time Zone Database or parsing/formatting dates/times with custom formats which are not relevant here and more effort to maintain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: Feature Type: New Features
Projects
None yet
Development

No branches or pull requests

3 participants