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

Differentiate From Ambiguous and Missing Dates For Timezone Conversion #840

Closed
daniel-nagy opened this issue Aug 22, 2020 · 10 comments
Closed
Labels

Comments

@daniel-nagy
Copy link

daniel-nagy commented Aug 22, 2020

It's not accurate to say that 2020-03-08T02:30:00 in America/Los_Angeles is ambiguous. It is actually nonexistent. I believe the two should be differentiated from. The API should provide enough context to know that the date was missing opposed to being ambiguous.

Let's look at some examples

Temporal.TimeZone
  .from("America/Los_Angeles")
  .getAbsoluteFor(Temporal.DateTime.from("2020-03-08T02:30:00"))
  .toString("America/Los_Angeles") // 2020-03-08T03:30-07:00[America/Los_Angeles]

Temporal.TimeZone
  .from("America/Los_Angeles")
  .getAbsoluteFor(Temporal.DateTime.from("2020-03-08T02:30:00"), { disambiguation: 'earlier' })
  .toString("America/Los_Angeles") // 2020-03-08T01:30-08:00[America/Los_Angeles]

Temporal.TimeZone
  .from("America/Los_Angeles")
  .getPossibleAbsolutesFor(Temporal.DateTime.from("2020-03-08T02:30:00")) // []

These are all very unexpected results and as a developer I don't have enough context to make sense of it. A better alternative would be to be explicit about what should happen

let dateTime = Temporal.DateTime.from("2020-03-08T02:30:00");

Temporal.TimeZone
  .from("America/Los_Angeles")
  .getAbsoluteFor(dateTime, { whenMissing: MissingDateChoice.RoundDown })
  .toString("America/Los_Angeles") // 2020-03-08T01:59:59.999-08:00[America/Los_Angeles]

Temporal.TimeZone
  .from("America/Los_Angeles")
  .getAbsoluteFor(dateTime, { whenMissing: MissingDateChoice.RoundUp })
  .toString("America/Los_Angeles") // 2020-03-08T03:00-07:00[America/Los_Angeles]

dateTime = Temporal.DateTime.from("2020-11-01T01:00:00");

Temporal.TimeZone
  .from("America/Los_Angeles")
  .getAbsoluteFor(dateTime, { whenAmbiguous: AmbiguousDateChoice.TakeFirst })
  .toString("America/Los_Angeles") // 2020-11-01T01:00-07:00[America/Los_Angeles]

Temporal.TimeZone
  .from("America/Los_Angeles")
  .getAbsoluteFor(dateTime, { whenAmbiguous: AmbiguousDateChoice.TakeSecond })
  .toString("America/Los_Angeles") // 2020-11-01T01:00:00-08:00[America/Los_Angeles]

IMO the compatible option for disambiguation is confusing and not very useful. I think it might also make sense to have the option to take, or round to, standard time or daylight time.

enum AmbiguousDateChoice {
  TakeFirst = "TAKE_FIRST",
  TakeSecond = "TAKE_SECOND",
  TakeDaylightTime = "TAKE_DAYLIGHT_TIME",
  TakeStandardTime = "TAKE_STANDARD_TIME"
}

enum MissingDateChoice {
  RoundDown = "ROUND_DOWN",
  RoundUp = "ROUND_UP",
  RoundToDaylightTime = "ROUND_TO_DAYLIGHT_TIME",
  RoundToStandardTime = "ROUND_TO_STANDARD_TIME"
}

A lower lever API can be added to explicitly get access to this information so the developer can make the best choice. This would essentially replace getPossibelAbsoluteFor

let choices = Temporal.TimeZone
  .from("America/Los_Angeles")
  .getAbsoluteChoicesFor(Temporal.DateTime.from("2020-08-22T10:30:00"));

assert choices = {
  ambiguous: false,
  choices: [
    Temporal.DateTime.from("2020-08-22T10:00:30-07:00")
  ]
  missing: false
}

let choices = Temporal.TimeZone
  .from("America/Los_Angeles")
  .getAbsoluteChoicesFor(Temporal.DateTime.from("2020-03-08T02:30:00"));

assert choices = {
  ambiguous: false,
  choices: [
    Temporal.DateTime.from("2020-03-08T01:59:59.999-08:00"),
    Temporal.DateTime.from("2020-03-08T03:00:00.000-07:00")
  ]
  missing: true
}

choices = Temporal.TimeZone
  .from("America/Los_Angeles")
  .getAbsoluteChoicesFor(Temporal.DateTime.from("2020-03-08T02:30:00"));

assert choices = {
  ambiguous: true,
  choices: [
    Temporal.DateTime.from("2020-11-01T01:00:00.000-07:00"),
    Temporal.DateTime.from("2020-11-01T01:00:00.000-08:00")
  ]
  missing: false
}

Note that choices is in chronological order. This API is inspired by the DateTime type in Elixir https://hexdocs.pm/elixir/DateTime.html#from_naive/3.

@ptomato
Copy link
Collaborator

ptomato commented Aug 24, 2020

Thanks for leaving the feedback!

Can you say more about what is missing from getPossibleAbsolutesFor that prevents it from being the lower-level API that you are describing? (Other than the naming distinction of ambiguous vs. missing?)

Unfortunately we cannot have an option to take the "standard" or "daylight" time, because those concepts don't exist in many time zones. In some, "standard" and "daylight" are the same, and there is a separate "winter" time. And in others, there are shifts in the UTC offset for other reasons than daylight saving.

The rationale for including compatible is that it is the existing behaviour of Date, as well as the Moment and Luxon libraries. (We originally didn't have compatible, so I kind of agree that it's not very useful, but we do expect that people will want to be able to keep the same behaviour when porting their existing codebases.)

@justingrant
Copy link
Collaborator

justingrant commented Aug 24, 2020

BTW, in addition to being the default for legacy Date, moment, date-fns, etc. the {disambiguation: 'compatible'} option is also required behavior of iCalendar, JSCalendar, and other related industry standards for calendar data.

From https://tools.ietf.org/html/rfc5545:

  If, based on the definition of the referenced time zone, the local
  time described occurs more than once (when changing from daylight
  to standard time), the DATE-TIME value refers to the first
  occurrence of the referenced time.  Thus, TZID=America/
  New_York:20071104T013000 indicates November 4, 2007 at 1:30 A.M.
  EDT (UTC-04:00).  If the local time described does not occur (when
  changing from standard to daylight time), the DATE-TIME value is
  interpreted using the UTC offset before the gap in local times.
  Thus, TZID=America/New_York:20070311T023000 indicates March 11,
  2007 at 3:30 A.M. EDT (UTC-04:00), one hour after 1:30 A.M. EST
  (UTC-05:00).

@daniel-nagy
Copy link
Author

Can you say more about what is missing from getPossibleAbsolutesFor that prevents it from being the lower-level API that you are describing? (Other than the naming distinction of ambiguous vs. missing?)

You're right, you could just check the length of getPossibleAbsolutesFor to detect if the date is missing or ambiguous.

Unfortunately we cannot have an option to take the "standard" or "daylight" time, because those concepts don't exist in many time zones. In some, "standard" and "daylight" are the same, and there is a separate "winter" time. And in others, there are shifts in the UTC offset for other reasons than daylight saving.

Interesting, I was curious if shifts could happen for reasons other than daylight savings.

The rationale for including compatible is that it is the existing behaviour of Date, as well as the Moment and Luxon libraries. (We originally didn't have compatible, so I kind of agree that it's not very useful, but we do expect that people will want to be able to keep the same behaviour when porting their existing codebases.)

That makes sense, thanks for the explanation!

@gilmoreorless
Copy link
Contributor

gilmoreorless commented Aug 25, 2020

To clarify some reasoning for the current naming and behaviour...

  1. You're correct that a time during the jump forward of clocks is non-existent. But it's also ambiguous.

    "Ambiguous" is generally used to mean "there's more than one interpretation / option", but it can also mean "vague and unclear" (ref). In this case, a non-existent time is unclear because it's invalid, but it also has more than one option for how to handle it: round to an earlier time, round to a later time, or throw an error. I think "disambiguation" still works as a name for this option.

  2. On the returning of date-time values that are one hour before or after the non-existing time, the key wording is in the RFC that @justingrant quoted:

    If the local time described does not occur (when
    changing from standard to daylight time), the DATE-TIME value is
    interpreted using the UTC offset before the gap in local times.

    The important part is that, even if the input time doesn't exist, it's still interpreted using a UTC offset that is valid for that time zone. This applies to both the "round to earlier" and "round to later" cases. This is far more likely to match the intent of the input.

    As others have noted, other standards and JS libraries have this behaviour. Many other programming languages do too, with the exception of Elixir as you've noted. I actually strongly disagree with Elixir's API in this regard, because it gives back what I consider invalid interpretations — especially the "1 millisecond before the jump" value. As I wrote in Disambiguation behaviour when converting DateTime skipped by DST transition #317 (comment):

    Imagine a task scheduled hourly in America/New_York with the equivalent UTC timestamps listed, using that process:

    America/New_York => UTC
    2020-03-08T00:45 => 2020-03-08T05:45
    2020-03-08T01:45 => 2020-03-08T06:45
    2020-03-08T02:45 => 2020-03-08T06:59:59.999999999  <--- Unexpected
    2020-03-08T03:45 => 2020-03-08T07:45
    

    If I saw that result in my application, I'd assume there was a bug somewhere.


Out of interest, did you happen to see the Ambiguity page in the documentation? It goes into a little more detail about this behaviour. If you didn't see it, that means we have a discoverability problem with the documentation. 😃

@ptomato @justingrant It might be worth making a "key concepts" page for the documentation and adding it prominently on the index page. That page could also contain a "which Temporal type should I use?" decision tree (which I swear I've seen here but can't find the issue — Edit: it's #655).

@daniel-nagy
Copy link
Author

daniel-nagy commented Aug 25, 2020

@gilmoreorless I agree that when adding 1 hour to 2020-03-08T01:45-05:00 EST the logical result would be 2020-03-08T01:45-04:00 EDT. IMO this is valid because you already have a valid date time in a specific timezone and you want to know what the time would be after adding some duration. In other words, you have enough context to do the right thing. In your example you're not converting a date time to an absolute you're just adding 1 hour to an absolute. If your example was not dealing with absolutes then you would end up with the same time twice when converting it to an absolute, which you might also interpret as a bug.

const dateTime = Temporal.DateTime.from("2020-03-08T02:45:00");

Temporal.TimeZone
  .from("America/New_York")
  .getAbsoluteFor(dateTime) // Temporal.Absolute <2020-03-08T07:45Z>

Temporal.TimeZone
  .from("America/New_York")
  .getAbsoluteFor(dateTime.add({hours: 1})) // Temporal.Absolute <2020-03-08T07:45Z>

In elixir, the behavior when adding 1 hour to a DateTime is as you would expect

DateTime.from_naive!(~N[2020-03-08T01:45:00], "America/New_York")
|> DateTime.add(60 * 60, :second) # DateTime<2020-03-08 03:45:00-04:00 EDT America/New_York>

I think the problem is when you receive a date time such as 2020-03-08T02:45 and the timezone "America/New_York" as input from some external source. If the input came from the user you would probably want to let them know the date time is invalid opposed to assuming they meant 2020-03-08T03:45 EDT. If you received this input from some other part of your application you may also assume there is a bug somewhere.

I don't think giving more context when converting a DateTime to an Absolute is incompatible or conflicting with your counter example.

Out of interest, did you happen to see the Ambiguity page in the documentation? It goes into a little more detail about this behaviour. If you didn't see it, that means we have a discoverability problem with the documentation. 😃

I did see it but I wasn't really satisfied with the solution which is why I created this issue. It could perhaps be more discoverable though.

@ptomato
Copy link
Collaborator

ptomato commented Aug 25, 2020

The Ambiguity and Balancing pages were already on the front page, but sort of buried in the Temporal.TimeZone and Temporal.Duration paragraphs respectively. See #848 for adding these to their own section.

@ptomato
Copy link
Collaborator

ptomato commented Aug 25, 2020

@gilmoreorless By the way: #655 is the flowchart issue you're looking for 😄

@gilmoreorless
Copy link
Contributor

Thanks @ptomato, I've edited my comment to link to it. Damn GitHub search — I looked for both "flow" and "chart" separately but "flowchart" didn't turn up. 😆

@gilmoreorless
Copy link
Contributor

I don't think giving more context when converting a DateTime to an Absolute is incompatible or conflicting with your counter example.

@daniel-nagy That's a fair point about addition and context. But I still think that it's incorrect for Elixir to return possible options that don't match any UTC offset used by the zone in question.

@ptomato
Copy link
Collaborator

ptomato commented Jan 14, 2021

I think we have clarified things in the documentation, so I'll close this issue, thanks for bringing it up!

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

No branches or pull requests

4 participants