Add icu_time::zone::utc_offset!

[sffc]

I generated this using gemini-cli.

Is there a reason we wouldn't want to do this?

[robertbastian]

Is there a reason we wouldn't want to do this?

Const-constructing UtcOffsets seems like an antipattern to me. We could similarly have time! or iso_date! macro constructors, but these should always be runtime values, which is why we don't. This is useful for testing, but I don't think it's a great API to expose otherwise.

[sffc]

Is there a reason we wouldn't want to do this?

Const-constructing UtcOffsets seems like an antipattern to me. We could similarly have time! or iso_date! macro constructors, but these should always be runtime values, which is why we don't. This is useful for testing, but I don't think it's a great API to expose otherwise.

It's not the same as time! at all, because the set of offsets is bounded, but the set of times is (virtually) unbounded.

I would expect utc_offset! to have usage patterns similar to a potential future time_zone!.

[sffc]

I consider gemini's changes fine, not bad enough to justify more cycles of iteration. I'll try feeding your feedback into the gemini agent though and see what happens.

[robertbastian]

I consider gemini's changes fine, not bad enough to justify more cycles of iteration.

Hard disagree. Gemini is changing code that does not need to be changed, and making code less readable. I'm not going to let it change code unnecessarily because you cannot be bothered to revert these changes.

[robertbastian]

It's not the same as time! at all, because the set of offsets is bounded, but the set of times is (virtually) unbounded.

Offsets are +-25 hours with second precision, times are 24 hours with nanosecond precision. You don't want to write out all consts for either of them.

I would expect utc_offset! to have usage patterns similar to a potential future time_zone!.

And what is the usage pattern for a potential future time_zone!? This is the first I'm hearing of that.

[sffc]

It's very common to write apps that hardcode their time zone, which often is OK an expected. If you are running a restaurant or a weather station or a TV channel or a train network, you will have a use case for time_zone! and/or utc_offset!.

[sffc]

I consider gemini's changes fine, not bad enough to justify more cycles of iteration.

Hard disagree. Gemini is changing code that does not need to be changed, and making code less readable. I'm not going to let it change code unnecessarily because you cannot be bothered to revert these changes.

If code A and code B produce the same output and adhere to policy, a PR should not be blocked because a reviewer feels A is better than B. This is a personal position that we need to discuss sometime in the TC.

I'm onboard if A and B compile to different bytecode. I haven't checked. It seems likely that they compile to the same, except perhaps for the unsigned_abs which Gemini should have been smart enough to keep.

[sffc]

I intend to push some code changes later when I'm back on the correct computer, but I'm still awaiting further discussion on whether utc_offset! is motivated to add.

[robertbastian]

If you are running a restaurant or a weather station or a TV channel or a train network, you will have a use case for time_zone! and/or utc_offset!.

If you're running a restaurant your opening hours will be trivially in local time, so you don't need to display a time zone at all. If you run any cross-timezone business ("a weather station or a TV channel or a train network"), you need to do time zone calculations. And if you do calculations, you should display the result of these calculations (date+time+zone), not display the date+time result with some hardcoded zone value.

This macro seems useful because it can be used in our docs in a few places. However, our docs manually construct datetime inputs, which is not what clients should do: they should convert the types that their calculations produce (IXDTF strings, chrono::DateTime, etc.), which does not need constants.

[robertbastian]

If code A and code B produce the same output and adhere to policy, a PR should not be blocked because a reviewer feels A is better than B. This is a personal position that we need to discuss sometime in the TC.

It's not that neither A or B exist and I'm forcing you to change from what you (or your AI) wrote to what I prefer. This is code that I wrote, that is checked in, that is released, that works, which your AI changed because it's too stupid, and which you don't want to revert because you cannot be bothered to do the work.

If code A is what has been written, reviewed, and checked in, a change to code B that behaves the same has absolutely no justification of being approved.

[sffc]

Maybe you are making a thermostat with 4 hardcoded choices of time zones for display purposes. It pulls the latest datetime-with-offset from a service.

[robertbastian]

Maybe you are making a thermostat with 4 hardcoded choices of time zones for display purposes. It pulls the latest datetime-with-offset from a service.

This is getting very hypothetical. If you have a thermostat with a choice of 4 time zones that talks to some external service, the external service will give you the offset. This goes back to my point that we format the offset that the timezone calculation code gives us.

[Manishearth]

If code A and code B produce the same output and adhere to policy, a PR should not be blocked because a reviewer feels A is better than B. This is a personal position that we need to discuss sometime in the TC.

I disagreed with this position back on the macro issue when we felt that it made the code much worse, however as I have previously stated I tend to consider "the PR author chose to write this" to be a strong point in favor of most code when the PR author is a team member, because someone whose judgement I trust felt that it was a good idea and spent time doing so. The fact that someone disagrees with the assesment is a point against, but in most cases I would find myself fine with limiting back and forth and just taking in the new code, unless I have strong readability reasons or whatever.

That's not happening here, though. Gemini is not someone whose code I trust. "Shane chose to write this code" is not a point in favor of this code because you did not choose to write this code, this code was presented as an option to you and you thought it was fine. That's quite different from sitting down and deciding which path to take.

[sffc]

ok I went through the rewritten function and made it more like what was there before, which was better.

[sffc]

I saw @robertbastian's PR and merged that instead.

[Manishearth]

Seems like we have a replacement API for this. That works for me, I do not plan to review this unless rerequested.

[sffc]

@robertbastian's PR is not a replacement for the API; it only makes the Rust function const so that this PR is only adding the macro and not touching pre-existing code.

[Manishearth]

Overall seems fine. Not convinced of the use case of constructing const offsets in code but this is a low cost simple API addition.

[robertbastian]

issue: I don't think we should use this macro in docs. users will have to create a UtcOffset from their sources, and they should know that UtcOffset::try_from_str exists for runtime values.

[sffc]

fixed

[robertbastian]

this applies to all docs

[sffc]

The docs aren't all homogeneous in this way. The docs here where you left your comment are on UtcOffset::try_from_str which should obviously not be using the macro (and I'll be scolding my agent for why it thought changing those docs was a good idea). But, most other examples are just hardcoding example time zones. Some already use macros for the time zone ID, like

        /// // Time zone info for America/Chicago in the summer
        /// let zone = TimeZone(subtag!("uschi"))
        ///     .with_offset(Some(utc_offset!("-05"))) // new
        ///     .at_date_time_iso(DateTime{ date: Date::try_new_iso(2022, 8, 29).unwrap(), time: Time::start_of_day() });

That's an improvement from what we had before: "-05".parse().ok() which silently threw away any error.

The macro also seems better than UtcOffset::try_from_seconds(-7 * 3600).unwrap().

Using the macro as the expectation in an assert_eq also seems like good design.

[robertbastian]

That's an improvement from what we had before: "-05".parse().ok() which silently threw away any error.

Yeah that should use Some("-05".parse().unwrap())

The macro also seems better than UtcOffset::try_from_seconds(-7 * 3600).unwrap().

No, because our users will have offsets in seconds or strings. This shows them how to use that value in our API. The macro does not.

[sffc]

If you hold this novel position on UtcOffset, do you also hold it on everywhere else we have macro helpers? Should we use Locale::try_from_str in most places where we use locale!, for example?

[robertbastian]

A user reading the docs should know that the locale!("fr") expression should be replaced by the actual user locale. They might try to replace it by locale!(user_local_str), which doesn't work – this is an issue, but a devex issue, not a correctness issue.

However, they might not realize that the utc_offset!("-03") expression needs to be replaced by the output of their time zone calculations. They might hardcode America/Punta_Arenas as one of the inputs to their time zone calculations, which is valid if they design a system that is local to Chile. However if they then also hardcode utc_offset!("-03") under the assumption that a TZ has a fixed offset, instead of the result of their time zone calculation, that is a correctness issue if Punta Arenas ever changes offset. Defining this macro and using it in docs is handing our clients a big footgun.

We have dozens of const constructors, but only have macros for a few locale types. I think it's fine to use locale! in docs (although not optimal, we should use something that lets readers discover the whole preferences architecture), because it doesn't require much i18n knowledge to know not to hardcode locales. It requires a lot of i18n knowledge to know not to hardcode time zone offsets.

[sffc]

ok I think this is landable from a technical point of view. We just disagree on the motivation.