sdk icon indicating copy to clipboard operation
sdk copied to clipboard

DateTime should have a copy constructor with value overwrite

Open pcornelissen opened this issue 9 years ago • 16 comments

I often have to create new DateTime object that are based on other DateTime objects. There are some methods like subtract and add which can create new instances, but sometimes you just want to have a new DateTime object at the same time but on a specific date or on the same day on another time.

For that I have created a small helper:

DateTime fromDateTime(
    DateTime src, {
    int year: null,
    int month : null,
    int day : null,
    int hour : null,
    int minute : null,
    int second : null,
    int millisecond : null}) {
  return new DateTime(
      year == null ? src.year : year,
      month == null ? src.month : month,
      day == null ? src.day : day,
      hour == null ? src.hour : hour,
      minute == null ? src.minute : minute,
      second == null ? src.second : second,
      millisecond == null ? src.millisecond : millisecond
  );
}

It would be really handy if there would be for example a constructor or a method on DateTime that could provide this.

Even better would be to keep the same timezone, but it's not a big deal for me, so I have not implemented in my solution yet.

pcornelissen avatar Oct 20 '15 17:10 pcornelissen

This could be a replace method on DateTime, like the one on Uri.

lrhn avatar Oct 21 '15 10:10 lrhn

The time zone issue is really vital, though. Creating the "same" time in a different time zone, invisibly, would be really bad. I don't think this can be put in until we have clear agreement on what the time zone and utc behavior should be, after thinking it through carefully.

whesse avatar Nov 11 '15 12:11 whesse

The version in the pull request also includes the isUTC flag.

pcornelissen avatar Nov 11 '15 12:11 pcornelissen

An internal Flutter customer (customer: fast) had an issue with their shipping app last week due to using DateTime.add(new Duration(days: 2)) not accounting for daylight savings. They were trying to move the day of a current DateTime. It's possible https://github.com/dart-lang/sdk/issues/27245 is the right solution, but this would have been another API which could have existed and helped them avoid the foot-gun of adding a Duration (and thus not having daylight savings time or other time oddities accounted for).

eseidelGoogle avatar Mar 22 '17 20:03 eseidelGoogle

The DataTime class is hampered by doing double duty as both a timestamp and a calendar object. It's doing at least one of those things ... less than optimally.

I think a solution could be to have a proper Date class as a library, one that doesn't bother with time-of-day or time zones, just (proleptic Gregorian) calendar days.

lrhn avatar Mar 28 '17 13:03 lrhn

We initially had different DateTime classes, but decided not to ship them in the core libraries. A complete Date library is complicated and we didn't want to ship too much code as part of the core libraries. (The same reasons explain, why Unicode support in the core libraries is limited).

If we wanted to be correct, then DateTime should be called ZonedDateTime, and we should have Date, DateTime, Zone classes.

I still think that having a simpler class in the core libraries is a good idea (although I would love to see a complete date library), but we should make it easier to avoid mistakes.

Note that the internal Google3 libraries provide a Date class.

floitschG avatar Mar 28 '17 13:03 floitschG

Java solves this problem by providing the Period class. Perhaps a similar approach could be taken in Dart?

camsteffen avatar May 22 '18 15:05 camsteffen

While the final decision does not come, I've created this extension to help:

extension DateTimeExtensions on DateTime {
  DateTime clone({
    int year: null,
    int month: null,
    int day: null,
    int hour: null,
    int minute: null,
    int second: null,
    int millisecond: null,
    bool isUtc: null,
  }) {
    if (isUtc == null ? this.isUtc : isUtc) {
      return DateTime.utc(
        year == null ? this.year : year,
        month == null ? this.month : month,
        day == null ? this.day : day,
        hour == null ? this.hour : hour,
        minute == null ? this.minute : minute,
        second == null ? this.second : second,
        millisecond == null ? this.millisecond : millisecond,
      );
    }
    return DateTime(
      year == null ? this.year : year,
      month == null ? this.month : month,
      day == null ? this.day : day,
      hour == null ? this.hour : hour,
      minute == null ? this.minute : minute,
      second == null ? this.second : second,
      millisecond == null ? this.millisecond : millisecond,
    );
  }
}

PS it works perfect with TZDateTime to work around the other _value private field bug.

NightOwlCoder avatar Jul 01 '20 03:07 NightOwlCoder

Extensions can work. I'd prefer something like interface default methods since they'd allow being virtual, and feels unnecessarily complicated to declare extension methods in the same library as the base type - but it's breaking to add methods to interfaces without interface default methods.

lrhn avatar Mar 11 '21 14:03 lrhn

This extension can now be simplified quite a bit with constructor tear-offs (Dart 2.17):

extension DateTimeExtensions on DateTime {
  DateTime copyWith({
    int? year,
    int? month,
    int? day,
    int? hour,
    int? minute,
    int? second,
    int? millisecond,
    int? microsecond,
    bool? isUtc,
  }) {
    return ((isUtc ?? this.isUtc) ? DateTime.utc : DateTime.new)(
        year ?? this.year,
        month ?? this.month,
        day ?? this.day,
        hour ?? this.hour,
        minute ?? this.minute,
        second ?? this.second,
        millisecond ?? this.millisecond,
        microsecond ?? this.microsecond,
    );
  }
}

spydon avatar Jun 10 '22 08:06 spydon

For folks who still have to write on Dart SDK < 2.15

extension DateTimeExtension on DateTime {
  DateTime copy() {
    if (isUtc) {
      return DateTime.utc(
        year,
        month,
        day,
        hour,
        minute,
        second,
        millisecond,
      );
    }
    return DateTime(
      year,
      month,
      day,
      hour,
      minute,
      second,
      millisecond,
    );
  }

  DateTime copyWith({
    int? year,
    int? month,
    int? day,
    int? hour,
    int? minute,
    int? second,
    int? millisecond,
    bool? isUtc,
  }) {
    if (isUtc ?? this.isUtc) {
      return DateTime.utc(
        year ?? this.year,
        month ?? this.month,
        day ?? this.day,
        hour ?? this.hour,
        minute ?? this.minute,
        second ?? this.second,
        millisecond ?? this.millisecond,
      );
    }
    return DateTime(
      year ?? this.year,
      month ?? this.month,
      day ?? this.day,
      hour ?? this.hour,
      minute ?? this.minute,
      second ?? this.second,
      millisecond ?? this.millisecond,
    );
  }
}

CengizhanParlak avatar Jun 10 '22 14:06 CengizhanParlak

Could we include DateTime.copyWith in Dart 3.0 (I can write up a PR if needed), since it is regarded as a breaking change? It is such a small feature, but it would make the code incredibly much nicer in many places that touch the DateTime API.

spydon avatar Sep 09 '22 07:09 spydon

We could make it an extension method, then it's not as breaking.

lrhn avatar Sep 09 '22 08:09 lrhn

Is it not worth adding it as a breaking change though? It has been discussed for like 8 years now.

spydon avatar Sep 09 '22 09:09 spydon

If it would be added as an extension, could it be in the same file as the DateTime class so that the user doesn't have to explicitly import the extension?

spydon avatar Sep 09 '22 09:09 spydon

I put up a draft PR, but maybe it is not needed now if #49928 will lead to that it is implemented as an instance member instead. EDIT: Seems like it should still be an extension in 49928. https://dart-review.googlesource.com/c/sdk/+/258541

spydon avatar Sep 09 '22 13:09 spydon

I don't think adding a copyWith method is a good idea. One of the things that everyone would assume for such a copy method is that dateTime.copyWith() == dateTime. However this is not always the case because on days where the clock is turned back, there are two moments in time with the same date and time of day. When construction a DateTime with these parameters dart chooses the first moment with matching parameters. So if the input is any DateTime within the second interval of duplicated time of days, this will automatically snap back to the fist occurence. And there is really no way of Propperly defining copyWith that avoids that problem.

Most of the time when people would want to use copyWith, they actually want something different. For example people might want to write the following methods:

extension on DateTime {
  // This might land you at a time more than an hour in the past in the second duplicated interval of a time change
  DateTime floorToSecond() => copyWith(millisecond: 0, microsecond: 0);

  // The day might not have a time of day the same as `this` if the time got put forward that day.
  // A better method should probably have a nullable return type to alert users to that possibility.
  DateTime sameTimeDifferentDay(DateTime day) =>
      copyWith(year: day.year, month: day.month, day: day.day);

  // Similar problem to sameTimeDifferentDay with times but also with February 29.
  // If `this` is in a leap year on February 29 and `year` is not a leap year,
  // then the result will be on March 1. This may or may not be what the user wants.
  // Again the return type should probably be nullable.
  DateTime inDifferentYear(int year) => copyWith(year: year);

  // Correct but verbose. User might miss microsecond when implementing like for example in the `copyWith` example implementations above AND IN THE PATCH!!!
  DateTime get startOfDay =>
      copyWith(hour: 0, minute: 0, second: 0, millisecond: 0, microsecond: 0);
}

Point is, each of these use cases has their own problems and requires their own solutions. So if anything, i think more high level methods should be added to DateTime.

Quijx avatar Sep 27 '22 19:09 Quijx

I'll say it here in a separate comment again for visibility: @spydon The PR seems to be missing microseconds. Just in case my previous comment is not convincing enough to not merge it :)

Quijx avatar Sep 27 '22 19:09 Quijx

So if the input is any DateTime within the second interval of duplicated time of days, this will automatically snap back to the fist occurrence. And there is really no way of properly defining copyWith that avoids that problem.

I think this is a trade-off that is worth taking, the users are already creating their own copyWith methods. Since pretty much all other modern datetime libraries have similar functionality, I'm thinking that all these other libraries and languages must have already done this trade-off in one way or another.

Point is, each of these use cases has their own problems and requires their own solutions. So if anything, i think more high level methods should be added to DateTime.

Creating high level methods for DateTime would be incredibly much harder, since we can't know what the users would want to do, and I don't think we could cover every possible mutation of DateTime nicely.

The PR seems to be missing microseconds.

Good catch, I'll add that!

spydon avatar Sep 27 '22 19:09 spydon

The comment about .copyWith() not necessarily being a clone is valid. Not sure it's actionable.

It's something we can consider worrying about and designing for. It's not clear what the perfect solution is, because it very much depends on the intent of the caller.

As stated, if you do .copyWith(seconds: 0, millseconds: 0, microseconds: 0) you probably want to truncate/round the time. We could just figure out the time delta then, and retain the current time zone. But if you do minutes:0, some places have non-whole-hour DST or other time adjustments, so we don't know whether you'll be in the same time zone or not.

We could try to do both: Use the verbatim result and the "adjust-for-time-difference" result, and if they end up in the same time zone, it's fine. If not, we need to figure out which one to use.

Or we can just tell people that using copyWith might give unexpected results around DST changes, just as any other date-time operaton

lrhn avatar Sep 28 '22 11:09 lrhn

Or we can just tell people that using copyWith might give unexpected results around DST changes, just as any other date-time operaton.

Since I don't think there is any perfect solution this is the best option imho.

spydon avatar Sep 28 '22 13:09 spydon

This is now merged and will be available from Dart 2.19! 🥳 https://dart-review.googlesource.com/c/sdk/+/258541

spydon avatar Oct 20 '22 10:10 spydon