sdk
sdk copied to clipboard
DateTime should have a copy constructor with value overwrite
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.
This could be a replace
method on DateTime
, like the one on Uri
.
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.
The version in the pull request also includes the isUTC flag.
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).
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.
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.
Java solves this problem by providing the Period
class. Perhaps a similar approach could be taken in Dart?
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.
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.
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,
);
}
}
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,
);
}
}
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.
We could make it an extension method, then it's not as breaking.
Is it not worth adding it as a breaking change though? It has been discussed for like 8 years now.
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?
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
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
.
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 :)
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 definingcopyWith
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!
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
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.
This is now merged and will be available from Dart 2.19! 🥳 https://dart-review.googlesource.com/c/sdk/+/258541