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

convert(to timeZone:) "feels wrong" for Absolute<Day> #34

Closed
sadlerjw opened this issue Mar 16, 2020 · 9 comments
Closed

convert(to timeZone:) "feels wrong" for Absolute<Day> #34

sadlerjw opened this issue Mar 16, 2020 · 9 comments

Comments

@sadlerjw
Copy link

convert(to timeZone:) behaves consistently for all T in Absolute<T> and in all other cases besides <Day> it behaves intuitively. For example, taking March 16 2020 at 12:00 PM EST (Absolute<Minute>) and converting it to PST gives you a different set of date components, which "feels" right.

Converting March 16, 2020 (Absolute<Day>) from UTC to NZDT (UTC+13) gives March 17, 2020 because internally it takes the approximate midpoint of March 16, 2020 UTC (noon on that day in UTC) and uses the date components of that instant in NZDT, which is 1:00AM March 17 NZDT. This "feels" wrong (to me).

The behaviour for anything smaller than Day is intuitively correct (as in the first example above), and for anything bigger than Day is intuitively correct because the midpoint of a month, year, or era will end up in the same month, year, or era (AFAIK, maybe this is one of those falsehoods programmers believe) because time zones are never several days apart from each other.

I suppose we can't change just the behaviour of convert(to timeZone:) for Absolute<Day> but I wonder if we could provide a sameDay(in timeZone:) or something?

This realization made me rethink the way I was doing things and in my own app this isn't actually something I need, so maybe things are fine as-is, or maybe convert(to timeZone:) just needs some docs explaining this?

@sadlerjw sadlerjw changed the title convert(to timeZone:) "feels wrong" for Absolute<Day> convert(to timeZone:) "feels wrong" for Absolute<Day> Mar 16, 2020
@AverageHelper
Copy link

It seems to me that a "time zone" matters more when a more granular hour or minute is involved, as opposed to only the calendrical day. March 16th is March 16th for half the world at a time; unless you're talking about a particular part of March 16th, which is different depending upon the time zone, one doesn't often consider the other time zones.

I do think this issue needs to be decided. It feels odd to, sans documentation, pick a time as arbitrary as noon o'clock and convert that to the other time zone. I like your idea of breaking the function apart for Absolute<Day>.

What comes to my mind is somehow disallowing convert(to timeZone:) to work for Absolute<Day> and larger, and instead force callers instead to use something like sameDay(in timeZone:) if they really want to use the middle-of-day logic.

I wonder too about Month and Year... how to calendrical conversions work? Can this package handle converting temporal moments from the Gregorian to whatever the Chinese calendar does?

@davedelong
Copy link
Owner

These are great points. I'll think about the proper way to solve this. I'm thinking it might involve falling back to a dateComponents → date approach.

@davedelong
Copy link
Owner

I was thinking about this a bit more. The current behavior is technically correct, and the confusion is arising from a bad API name (and please LMK if you disagree).

The conversion methods, as they currently stand, are about converting temporal coordinates from one localized representation to another. The most reliable way of doing that is to convert based off the approximate midpoint of the temporal range, and use that as the base value for conversion. In other words, the temporal range of 1 Jan 2000 UTC overlaps the most with the temporal range of 2 Jan 2000 NZT.

There is a different kind of conversion, which is "I have this set of calendar components, and I want to get a calendar value that has these same components but in another timezone". In other words, "I have {day: 1, month: 1, year: 1} in UTC. I want to get a value that means {day: 1, month: 1, year: 1} in NZT".

For most calendar units, these two kinds of conversions are largely equivalent. But when we get down to the smaller units, we get tripped up on the distinction between these two.

Time already has code to do both kinds of conversion, but they're not well-named and not entirely exposed.

What suggestions do you have on naming these kinds of conversions, and how would you like to see them differentiated from each other?

@davedelong
Copy link
Owner

My current thinking on the names are today.convertingTimeRange(to: ...) and today.convertingComponents(to: ...)

@AverageHelper
Copy link

That sounds good to me. As long as the documentation is explicit about the differences and quirks between them (I'm okay reading well-written documentation paragraphs) I'm okay with those signatures.

@AverageHelper
Copy link

Although, "time range" feels a bit clunky, since that phrase isn't common parlance in the package.

@sadlerjw
Copy link
Author

Are there use cases for convertingTimeRange for Absolute<T> where T : GTOEDay? I wonder if we could restrict convertingTimeRange(to: ...) to smaller values and making convertingComponents(to: ...) available to all Absolutes?

Alternatively, I wonder if the same-components-with-different-time-zone should actually be considered a "conversion"...what do you think of today.with(timeZone:) or today.changingTimeZone(to:) or something? (In which case the existing convert(to:) methods wouldn't necessarily need to be renamed)

@davedelong
Copy link
Owner

davedelong commented Feb 21, 2024

I believe I've addressed this in the latest code, here: https://github.com/davedelong/time/blob/main/Sources/Time/4-Fixed%20Values/Fixed%2BConversion.swift#L40

There are targeted methods for converting Absolute Fixed values, based on their granularity. For the most flexibility, there's also a a general converted(to:behavior:) method where you can specify a new region/timezone/calendar, as well as the behavior of the conversion: should it work by preserving the underlying date components, or should it work by preserving the underlying time range? This method throws an error if it can't guarantee correctness.

This will be included in the upcoming 1.0 release.

@davedelong
Copy link
Owner

This shipped in 1.0. There are a couple alternatives:

  • All Fixed values can convert to new Locales without issue, since that only affects how the value is formatted
  • fixed date values (Fixed<Era>, Fixed<Year>, Fixed<Month>, and Fixed<Day>) can convert between time zones and will throw an error if it fails. This preserves components and will fail if the components do not exist in the new time zone.
  • fixed time values (Fixed<Hour>, Fixed<Minute>, Fixed<Second>, and Fixed<Nanosecond>) can convert between time zones. This conversion preserves the range and will fail if the converted value does not represent the same Range<Instant> as the original value
  • fixed days and time values (Fixed<Day>, Fixed<Hour>, Fixed<Minute>, Fixed<Second>, and Fixed<Nanosecond>) can convert between calendars, because all calendars tend to have the same instantaneous boundaries for these units. This conversion preserves the range.
  • All fixed values can convert to a new Region by specifying a desired behavior (preserving components or preserving ranges). This will throw an error if the behavior cannot be guaranteed. This technically allows for some nonsensical conversions, but the thrown error will make that clear.

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

No branches or pull requests

3 participants