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

Add nextTransitions and previousTransitions functionality to ZonedDateTime #1005

Open
nordzilla opened this issue Aug 25, 2021 · 3 comments
Open
Assignees
Labels
C-time-zone Component: Time Zones help wanted Issue needs an assignee S-medium Size: Less than a week (larger bug fix or enhancement) T-core Type: Required functionality

Comments

@nordzilla
Copy link
Member

nordzilla commented Aug 25, 2021

For a ZonedDateTime, we need the ability to tell when the next and previous daylight savings time transitions are.

This interface should return an iterator over the transitions.

fn nextTransitions(_) -> impl Iterator<Item = _>;
fn previousTransitions(_) -> impl Iterator<Item = _>;

Because not all time zones participate in DST and may have no transitions. Also, a ZonedDateTime may be associated with only a GMT offset (not a named time zone), in which case it would have also have no transitions.

This depends on being able to look up the time-zone data from the IANA time-zone database.

For more context see

@nordzilla nordzilla added C-datetime Component: datetime, calendars, time zones help wanted Issue needs an assignee S-medium Size: Less than a week (larger bug fix or enhancement) T-core Type: Required functionality labels Aug 25, 2021
@sffc
Copy link
Member

sffc commented Aug 25, 2021

I don't think we need an iterator; the spec says

  • 11.4.9 Temporal.TimeZone.prototype.getNextTransition ( startingPoint )
  • 11.4.10 Temporal.TimeZone.prototype.getPreviousTransition ( startingPoint )

In both cases, startingPoint is an Instant (timestamp).

@nordzilla
Copy link
Member Author

I don't think we need an iterator; the spec says

* 11.4.9 Temporal.TimeZone.prototype.getNextTransition ( startingPoint )

* 11.4.10 Temporal.TimeZone.prototype.getPreviousTransition ( startingPoint )

In both cases, startingPoint is an Instant (timestamp).

So my initial thought process was to do something more like what you just said, but in the Time Zones Deep Dive (Part 3) I recall we had decided that an iterator API would be nice:

Shane: The time zone format and time zone object should be able to support throwing any timestamp at it, and it is able to format it. The time zone doesn't necessarily know where it sits. Yes, it's definitely rule based. A time zone is a list of transitions. It's a list represented by generators or rule based, but it's an implementation detail.

Richard: I think the clarification is good. The observable is a sequence of transitions. Disagree that you can just cut it off. You've got a functionally different time zone if you do this approach.

Justin: I agree with Richard; my point was mainly from an end-user standpoint. The other thing is that I think an iterator-style API is sufficient for use cases; there's no need to build an API that returns a full list.

Independent of the iterator vs. single-next-item question is whether this functionality should live on the TimeZone object or the ZonedDateTime object. I'm happy to move it to the TimeZone object if there is a use case for getting next and/or previous transitions from multiple different starting dates, but I also thought it was clean to have it on the ZonedDateTime object where the starting date is implicitly the date of the DateTime itself.

So we have two questions to resolve:

  1. Iterator (lazy) vs. single next transition.
  2. This functionality is on TimeZone, on ZonedDateTime, or both possibly for convenience.

I quite like the idea of the iterator API. Given the lazy nature of iterators, I wouldn't expect it to be much more expensive (if at all) than returning only the next item from the start date, and people could use it to get all of them in sequence if desired.

@nordzilla nordzilla added this to the ICU4X 0.5 milestone Aug 26, 2021
@sffc
Copy link
Member

sffc commented Aug 27, 2021

An iterator API seems reasonable if you can make it efficient as you described.

The reason why I think this function belongs on TimeZone and not ZonedDateTime is because of the data model of the two classes. In ECMAScript Temporal, ZonedDateTime is a bag with three fields (TimeZone, Calendar, and Instant). TimeZone is the thing that contains all information relating to the rules about where transitions occur and what the offsets should be. So putting this method on ZonedDateTime would just involve delegating to TimeZone anyway. We could have it on both, but I don't see how we would have it only on ZonedDateTime and not on TimeZone.

@nordzilla nordzilla self-assigned this Jan 20, 2022
@nordzilla nordzilla modified the milestones: ICU4X 0.5, ICU4X 1.0 Jan 20, 2022
@sapriyag sapriyag modified the milestones: ICU4X 1.0, ICU4X 1.1 May 25, 2022
@sffc sffc added C-time-zone Component: Time Zones and removed C-datetime Component: datetime, calendars, time zones labels Dec 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-time-zone Component: Time Zones help wanted Issue needs an assignee S-medium Size: Less than a week (larger bug fix or enhancement) T-core Type: Required functionality
Projects
None yet
Development

No branches or pull requests

3 participants