Skip to content
This repository has been archived by the owner on Nov 3, 2018. It is now read-only.

Consider how to advance across a DST transition #34

Closed
mattjohnsonpint opened this issue Jul 1, 2015 · 12 comments
Closed

Consider how to advance across a DST transition #34

mattjohnsonpint opened this issue Jul 1, 2015 · 12 comments

Comments

@mattjohnsonpint
Copy link
Owner

There's currently no clean way to advance a DateTime or a DateTimeOffset across a DST transition and end up in the right place. Either time will be off, or offset, or both.

A DateTimeZoned type would be one way to handle this.

Another way would be via method overloads that take a time zone, such as in the following example:

TimeZoneInfo tz = TimeZoneInfo.FindSystemTimeZoneById("Pacific Standard Time");
DateTimeOffset dto = new DateTimeOffset(2015, 3, 8, 1, 0, 0, TimeSpan.FromHours(-8));
DateTimeOffset result = dto.AddHours(2, tz); // 2015-03-08T04:00:00-07:00

This would be clear for DateTimeOffset, it's not as clear for DateTime, due to potential conflicts between TimeZoneInfo and DateTimeKind.

DateTime result = DateTime.UtcNow.AddHours(2, TimeZoneInfo.Local);
// Does result have DateTimeKind.Utc or DateTimeKind.Local or does this throw an exception?

Also consider what should be done if the input kind is Unspecified, but falls into an invalid or ambiguous value for the given time zone.

One possible solution is that these extension methods only exist on DateTimeOffset.

@mattjohnsonpint
Copy link
Owner Author

I suppose this could also be written as any of the following:

1)  DateTimeOffset result = dto.AddHours(2, tz);
2)  DateTimeOffset result = tz.AddHours(dto, 2);
3)  DateTimeOffset result = tz.Add(dto, TimeSpan.FromHours(2));
4)  DateTimeOffset result = TimeZoneInfo.AddHours(dto, 2, tz);
5)  DateTimeOffset result = TimeZoneInfo.Add(dto, TimeSpan.FromHours(2), tz);

Please vote by number of which feels most natural to you. Thanks.

@HaloFour
Copy link

HaloFour commented Jul 2, 2015

Given the choices I kind of prefer the extension method off of DateTimeOffset. What exactly would happen if the offset doesn't match that of the TimeZoneInfo, though? Or if the time and offset represents a time that wouldn't be legal in that time zone due to a DST transition?

The DateTime.Utc question is an interesting one. Would the value be converted to the specified time zone, then changed, then converted back to UTC? As for the Kind of the result, I think it would have to be UTC as what if the TimeZoneInfo isn't local? As for Unspecified, I might treat that as the wall clock in the specified TimeZoneInfo and throw if the time is either illegal or ambiguous.

I think that these are good ideas but don't replace actually implementing additional proper date/time/tz types and behaviors.

@mattjohnsonpint
Copy link
Owner Author

Moving ahead with the first option for now. I can change it later if we decide to go that route. See #35 for implementation (in progress).

@mattjohnsonpint
Copy link
Owner Author

@HaloFour - for now, I'll focus on the DateTimeOffset ones, since there are fewer questions there. We can add them for DateTime if we can figure out how we want to handle the various issues.

@mattjohnsonpint
Copy link
Owner Author

After implementing the basic methods, I realized that it's not that much more work to allow for user-defined "resolvers", similar to Noda Time's ZoneLocalMappingResolvers.

This is advantageous because it lets us define a default behavior for dealing with gaps and overlaps without locking the user down to a single behaviior.

See the PR for the implementation.

@mattjohnsonpint
Copy link
Owner Author

@HaloFour to answer your questions:

What exactly would happen if the offset doesn't match that of the TimeZoneInfo?

There's a difference between the date-centric methods (AddYears, AddMonths, AddDays) and the time-centric methods (AddHours, AddMinutes, AddSeconds, AddMilliseconds, AddTicks, Add(TimeSpan), Subtract(TimeSpan)).

With the time-centric methods, it wouldn't matter if the offset matched or not. Essentially, it would have the same result as if the value was converted to UTC, the time was added there, and then it was converted to the specified time zone.

However, with the date-centric methods, it wouldn't make much sense to take in a DateTimeOffset without considering its offset - so I believe the right thing to do would be:

  1. Convert the input datetimeoffset to the equivalent instant in the provided time zone
  2. Add the years/months/days to the datetime part
  3. Use the resolver to determine the final datetimeoffset.

I've updated the PR to include step 1, which I was missing before.

@mattjohnsonpint
Copy link
Owner Author

Continuing the rest of the questions:

Or if the time and offset represents a time that wouldn't be legal in that time zone due to a DST transition?

If the input time isn't valid, that's ok because it's adjusted as described earlier. If the output time isn't valid, or is ambiguous, that's when the resolver has value. The default resolver uses the same behavior as Java 8, and as Noda Time 2.0 (Noda Time 1.x had a different default behavior. See "Lenient resolver changes" at the bottom of this documentation). The behavior is as follows:

  • Ambiguous times choose the earlier of the two possible values.
  • Invalid times skip forward by the amount of the transition gap (usually 1 hour), which is the same instant as if the transition had not occurred.

Again, these behaviors can be overridden by the user.

The DateTime.Utc question is an interesting one. Would the value be converted to the specified time zone, then changed, then converted back to UTC?

Well, if I implement the same kind of thing with DateTime, then I would expect it would follow the same 3 steps outlined earlier. That would mean that the input value would be converted before adding, but the output would be in the specified zone, with DateTimeKind.Unspecified.

Or - we could have it follow the same steps, but always return a DateTimeOffset, even if you started with a DateTime. What do you think about that?

@mattjohnsonpint
Copy link
Owner Author

Ok, I went ahead and added what I think is the correct implementation for DateTime to the PR. Please let me know your thoughts. Thanks.

@Clockwork-Muse
Copy link

Just adding a note to document this currently leaves System.Globalization.Calendar and subclasses out in the cold. May not be something we need to worry about, and this use case is covered by NodaTime, too.

@mattjohnsonpint
Copy link
Owner Author

@Clockwork-Muse - That's interesting when thinking about moving by months or years, since those are very-much specific to calendar.

@mattjohnsonpint
Copy link
Owner Author

The methods on System.Calendar handle this, but it might be worthwhile to extend them via extension methods. Alternatively, we could continue to expand the extensions for DateTime and DateTimeOffset to take Calendar parameters. Perhaps we should do both?

@mattjohnsonpint
Copy link
Owner Author

Merged the PR and closing this for now. We'll pick up the conversation after the migration to corefxlabs. We can either continue down this path, or backtrack if so decided. Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants