Skip to content

Add ZoneNameTimestamp::from_epoch_seconds#7720

Merged
robertbastian merged 2 commits intounicode-org:mainfrom
robertbastian:epochsecs
Mar 10, 2026
Merged

Add ZoneNameTimestamp::from_epoch_seconds#7720
robertbastian merged 2 commits intounicode-org:mainfrom
robertbastian:epochsecs

Conversation

@robertbastian
Copy link
Member

@robertbastian robertbastian commented Feb 27, 2026

The current API requires constructing a ZonedDateTime from UNIX seconds, and then internally converts it back to UNIX seconds.

Split from #7630

Changelog

icu_time: Add ZoneNameTimestamp::from_epoch_seconds()

  • New methods: ZoneNameTimestamp::from_epoch_seconds()

@robertbastian robertbastian force-pushed the epochsecs branch 4 times, most recently from ed4cce0 to 73f491b Compare February 27, 2026 16:39
Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah that does look cleaner

sffc
sffc previously requested changes Feb 27, 2026
Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue: This makes the tests and docs significantly less readable.

I'm in favor of adding the constructor but not using it as a replacement for the ISO calendar dates in tests and docs.

@robertbastian
Copy link
Member Author

it makes them significantly more realistic

you proposed this method

@robertbastian robertbastian requested a review from sffc February 27, 2026 22:20
Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My comment is not resolved

@robertbastian robertbastian requested a review from sffc March 2, 2026 10:00
@robertbastian
Copy link
Member Author

I've responded to your comment

@robertbastian
Copy link
Member Author

I'm in favor of adding the constructor but not using it as a replacement for the ISO calendar dates in tests and docs.

I'm trying to find a compromise here. You didn't like using non-ISO calendars, so this is the compromise, and it's actually the best solution. There are four ways to document TimeZoneInfo<AtTime> construction:

  • .with_zone_name_timestamp(ZoneNameTimestamp::from_epoch_seconds(s)): this is the most performant way, because you will already have the timestamp from your time zone arithmetic code. For example, jiff::Zoned::timestamp is just a field lookup.
  • .at_date_time(dt): this is nice when you want to demonstrate that the timestamp is some human-readable date, for which we should use "human" calendars. It's not performant, as we have to internally convert the date to an RD, and the RD to a timestamp.
  • .at_date_time_iso: this has the same performance drawbacks as at_date_time, but suggest that this is a performant "machine" input, which it's not (the timestamp is).
  • .with_zone_name_timestamp(ZoneNameTimestamp::far_in_future()): this demonstrates that the ZoneNameTimestamp has to be set, without showing how to set it from the result of time zone arithmetic. The tests will pass, but it's not a good example.

@sffc
Copy link
Member

sffc commented Mar 4, 2026

A fifth option is

.with_zone_name_timestamp(
    ZoneNameTimestamp::from_epoch_seconds(
        ZonedDateTime::try_from_str("2026-03-04T12:00Z", Iso).unwrap().to_epoch_seconds()))

and a sixth

.with_zone_name_timestamp(zone_name_timestamp!("2026-03-04T12:00Z"))  // or something similar

A seventh would be to use a third-party library to get the timestamp if you find ZonedDateTime untasteful to use.

@robertbastian
Copy link
Member Author

We don't currently have ZonedDateTime<C, UtcOffset>::to_epoch_seconds, and I don't want to add it, because it adds timezone calculcation logic to ICU4X that is only available for fixed offsets, but not for actual time zones.

@robertbastian
Copy link
Member Author

And re the macro, I'll point out the same thing I pointed out when you wanted to add a utc_offset! macro: this is a bad example because it's essentially only useable in docs when you have a constant date. Clients will then have to go into the docs and figure out what type with_zone_name_timestamp accepts, and how to create that. Using .with_zone_name_timestamp(ZoneNameTimestamp::from_epoch_seconds(12345)) makes it very clear what to plug in instead of 12345.

@sffc
Copy link
Member

sffc commented Mar 4, 2026

.with_zone_name_timestamp(zone_name_timestamp!("2026-03-04T12:00Z")) serves the purpose of demonstrating that a ZoneNameTimestamp is the correct type, and then you can go read the docs on that type to figure out how to use it.

@robertbastian
Copy link
Member Author

robertbastian commented Mar 4, 2026

It's not just that ZoneNameTimestamp is the correct type, it's that constructing it from a timestamp is the correct constructor.

@robertbastian
Copy link
Member Author

I also think it's bad to put actual dates, because that can be understood as if the date is used to determine whether a timezone formats as "standard" or "daylight", which is not the case. We currently set winter dates for winter timezones, and summer date for summer timezones. This doesn't happen with an opaque timestamp.

@sffc
Copy link
Member

sffc commented Mar 4, 2026

Then add ZoneNameTimestamp::middle_of_year(year) and ZoneNameTimestamp::start_of_year(year)

@robertbastian
Copy link
Member Author

What does that solve?

@sffc
Copy link
Member

sffc commented Mar 4, 2026

Then add ZoneNameTimestamp::middle_of_year(year) and ZoneNameTimestamp::start_of_year(year)

That doesn't really make a lot of sense either, though. You are allowed to use a winter ZoneNameTimestamp to display a time in the summer.

@robertbastian
Copy link
Member Author

The way I see these examples it that they are primarily for libraries to integrate with icu_datetime. Nobody is producing these values from thin air, there will be a timezone library. So we should show how to correctly integrate a timezone library with this type, which is to use ::from_epoch_seconds.

Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't agree with your choices on documentation but you explained your perspective and they seem like something a reasonable person could arrive at.

@robertbastian robertbastian merged commit 0f9ba47 into unicode-org:main Mar 10, 2026
33 checks passed
@robertbastian robertbastian deleted the epochsecs branch March 10, 2026 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants