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

DateTime should have a copy constructor with value overwrite #24644

Closed
pcornelissen opened this issue Oct 20, 2015 · 22 comments
Closed

DateTime should have a copy constructor with value overwrite #24644

pcornelissen opened this issue Oct 20, 2015 · 22 comments
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. core-2 library-core type-enhancement A request for a change that isn't a bug

Comments

@pcornelissen
Copy link

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.

@floitschG floitschG added area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-core labels Oct 21, 2015
@lrhn
Copy link
Member

lrhn commented Oct 21, 2015

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

@whesse
Copy link
Contributor

whesse commented Nov 11, 2015

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.

@pcornelissen
Copy link
Author

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

@eseidelGoogle
Copy link

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 #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).

@lrhn
Copy link
Member

lrhn commented Mar 28, 2017

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.

@floitschG
Copy link
Contributor

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.

@camsteffen
Copy link
Contributor

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

@NightOwlCoder
Copy link

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.

@lrhn lrhn added type-enhancement A request for a change that isn't a bug enhancement-breaking-change An enhancement which is breaking. labels Oct 5, 2020
@lrhn
Copy link
Member

lrhn commented Mar 11, 2021

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.

@spydon
Copy link
Contributor

spydon commented Jun 10, 2022

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,
    );
  }
}

@CengizhanParlak
Copy link

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,
    );
  }
}

@spydon
Copy link
Contributor

spydon commented Sep 9, 2022

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.

@lrhn
Copy link
Member

lrhn commented Sep 9, 2022

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

@spydon
Copy link
Contributor

spydon commented Sep 9, 2022

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

@spydon
Copy link
Contributor

spydon commented Sep 9, 2022

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
Copy link
Contributor

spydon commented Sep 9, 2022

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

@Quijx
Copy link

Quijx commented Sep 27, 2022

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
Copy link

Quijx commented Sep 27, 2022

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 :)

@spydon
Copy link
Contributor

spydon commented Sep 27, 2022

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!

@lrhn
Copy link
Member

lrhn commented Sep 28, 2022

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

@spydon
Copy link
Contributor

spydon commented Sep 28, 2022

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.

copybara-service bot pushed a commit that referenced this issue Oct 20, 2022
This change has been discussed for 8+ years and it would of course be preferred
to be added as an instance method, but since that is a breaking change I added it as an extension as discussed here:
#24644 (comment)

Change-Id: Iebb9f300e449920ae8891abac88f30b271321661
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/258541
Commit-Queue: Lasse Nielsen <lrn@google.com>
Reviewed-by: Nate Bosch <nbosch@google.com>
Reviewed-by: Lasse Nielsen <lrn@google.com>
@spydon
Copy link
Contributor

spydon commented Oct 20, 2022

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

@lrhn lrhn removed the enhancement-breaking-change An enhancement which is breaking. label Oct 20, 2022
@lrhn lrhn closed this as completed Oct 20, 2022
jupblb added a commit to micromentor-team/mentoring-flutter-app that referenced this issue May 26, 2023
There is a copyWith method available in the upstream since Dart 2.19:
dart-lang/sdk#24644
jupblb added a commit to micromentor-team/mentoring-flutter-app that referenced this issue May 31, 2023
There is a copyWith method available in the upstream since Dart 2.19:
dart-lang/sdk#24644
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. core-2 library-core type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

10 participants