Respect length in zone fieldsets #6029

robertbastian · 2025-01-23T17:09:08Z

Z, V, and O are all capable of formatting multiple lengths, but currently always format long. This adds the relevant constructors and integrates with the fieldset builder.

I've also replaced with_zone_specific_long() et al by a with_zone(impl ZoneMarkers), as otherwise the API would have exploded:

fieldsets::T::medium().with_zone(Z::short());
fieldsets::YMDT::long().with_zone(V::long());
fieldsets::YMDT::long().with_zone(Vs::short()); // only has a short constructor
fieldsets::YMDT::long().with_zone(L::new()); // only has a single length

ffi/dart/test/icu_test.dart

Manishearth

Code looks fine, but I believe @sffc bounced back and forth on some designes around this, so I'll let him decide

sffc · 2025-01-23T17:27:32Z

Issue: this looks like it basically undoes much of what I implemented in #5936, which was the result of multiple tradeoffs and I believe landed at the best overall design. I would have really appreciated a discussion about this before you went and implemented a change to it.

ffi/dart/test/icu_test.dart

robertbastian · 2025-01-23T17:35:56Z

It looks like there was also no discussion on #5936, you just went ahead and implemented something.

You added the short data to the Z and V fieldsets, but it's currently inaccessible. This makes it accessible.

components/datetime/src/builder.rs

sffc · 2025-01-23T17:42:58Z

components/datetime/src/fieldsets.rs

-            pub fn with_zone_location(self) -> Combo<Self, L> {
-                Combo::new(self, L::new())
+            /// Associates this field set with a time zone format.
+            pub fn with_zone<Z: ZoneMarkers>(self, zone: Z) -> Combo<Self, Z> {


Praise: This new signature for with_zone is nice.

One concern I thought of with regard to this signature. Currently, users do not typically need to ever write out the Z, Vs, or other time zone field set types. This new signature requires that they do this. With the old signature, user code ends up being the comparatively more readable with_zone_specific as opposed to with_zone(Zs::short()).

sffc · 2025-01-23T17:45:38Z

components/datetime/src/fieldsets.rs

@@ -833,11 +795,9 @@ macro_rules! impl_zone_marker {
        // A plain language description of the field set for documentation.
        description = $description:literal,
        // Length of the skeleton if this is the only field.
-        length_override = $length_override:ident,
+        $(sample_length = $sample_length:ident,)?


Issue: Adding back sample_length here means that the field set gains a length field, which is the same length field as is used by other fields. Month and Time Zone Name lengths should be independently settable.

these macros are way too complex. this works

No, this doesn't work. The presence of sample_length causes the length field to be added. The length field should be inoperable for time zone field sets if the length derives from the type, as it has done since I landed my change.

The point of this PR is adding the length field to time zone field sets.

sffc · 2025-01-23T17:48:00Z

components/datetime/src/fieldsets.rs

-// TODO: Add short/long UTC offset?
+impl_marker_length_constructors!(O,);
+impl O {
+    pub(crate) fn to_field(self) -> (fields::TimeZone, fields::FieldLength) {


Issue: to_field can be done in the macro. Previously I wrote it as a switch statement between long and non-long, with the two results being arguments to the macro.

I didn't see a way to do this. Zs, Vs, Z, V, and O have clear mappings between the Length and the FieldLength, but L and X don't have a Length and produce different field length. This code is very easy to understand, wrapping it in multiple macros would not be.

sffc · 2025-01-23T17:49:09Z

You added the short data to the Z and V fieldsets, but it's currently inaccessible. This makes it accessible.

I noticed this yesterday and it is a bug. Z previously contained short data and it was reachable through code similar to what is in this PR, but that code is gone, and short data should be removed from Z.

sffc · 2025-01-23T17:51:43Z

It looks like there was also no discussion on #5936, you just went ahead and implemented something.

I could have left more breadcrumbs about why I landed on that design, but asking on that PR why I did things a certain way, even after the PR landed, would have been a good thing to do.

robertbastian · 2025-01-23T18:03:29Z

short data should be removed from Z.

Says you. How I remember the design that was discussed was that Z and V support both lengths (like all other field sets support all lengths), and Zs and Vs were escape hatches to avoid including long data.

sffc · 2025-01-23T18:05:05Z

components/datetime/src/fieldsets.rs

@@ -833,11 +795,9 @@ macro_rules! impl_zone_marker {
        // A plain language description of the field set for documentation.
        description = $description:literal,
        // Length of the skeleton if this is the only field.
-        length_override = $length_override:ident,
+        $(sample_length = $sample_length:ident,)?


No, this doesn't work. The presence of sample_length causes the length field to be added. The length field should be inoperable for time zone field sets if the length derives from the type, as it has done since I landed my change.

sffc · 2025-01-23T18:07:17Z

short data should be removed from Z.

Says you. How I remember the design that was discussed was that Z and V support both lengths (like all other field sets support all lengths), and Zs and Vs were escape hatches to avoid including long data.

Yes, and I altered that design in #5936, while you were away. I'm happy to reopen that discussion if there was something wrong with the decision I made, but it's awkward to do it on a PR that basically reverts #5936.

#5936 should have removed the short data and it is a bug that it didn't.

sffc · 2025-01-24T20:34:39Z

It's not ideal that this PR was opened at this time since I am currently making large-ish changes to some of these files (especially builder.rs and in icu_capi) to update the datetime FFI. I intend to continue with those changes even though it will cause a merge conflict.

In the future, we should discuss large/disruptive changes before we spend time on them.

Respect length in zone fieldsets

5ff16e4

robertbastian requested review from Manishearth, sffc, zbraniecki and a team as code owners January 23, 2025 17:09

robertbastian commented Jan 23, 2025

View reviewed changes

ffi/dart/test/icu_test.dart Outdated Show resolved Hide resolved

Manishearth removed request for a team, zbraniecki and Manishearth January 23, 2025 17:18

Manishearth reviewed Jan 23, 2025

View reviewed changes

fix

f437a9b

sffc requested changes Jan 23, 2025

View reviewed changes

ffi/dart/test/icu_test.dart Outdated Show resolved Hide resolved

fix

c628634

robertbastian requested a review from sffc January 23, 2025 17:35

sffc reviewed Jan 23, 2025

View reviewed changes

components/datetime/src/builder.rs Outdated Show resolved Hide resolved

revert diplomat

b4e9bfc

sffc requested changes Jan 23, 2025

View reviewed changes

restore ZoneStyle variants

fc0d3d7

robertbastian requested a review from sffc January 23, 2025 17:58

diplomat

96a49b3

sffc requested changes Jan 23, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Respect length in zone fieldsets #6029

Respect length in zone fieldsets #6029

robertbastian commented Jan 23, 2025

Manishearth left a comment

sffc commented Jan 23, 2025

robertbastian commented Jan 23, 2025

sffc Jan 23, 2025

sffc Jan 24, 2025

sffc Jan 23, 2025

robertbastian Jan 23, 2025

sffc Jan 23, 2025

robertbastian Jan 23, 2025

sffc Jan 23, 2025

robertbastian Jan 23, 2025

sffc commented Jan 23, 2025

sffc commented Jan 23, 2025

robertbastian commented Jan 23, 2025

sffc Jan 23, 2025

sffc commented Jan 23, 2025 •

edited

Loading

sffc commented Jan 24, 2025

Respect length in zone fieldsets #6029

Are you sure you want to change the base?

Respect length in zone fieldsets #6029

Conversation

robertbastian commented Jan 23, 2025

Manishearth left a comment

Choose a reason for hiding this comment

sffc commented Jan 23, 2025

robertbastian commented Jan 23, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sffc commented Jan 23, 2025

sffc commented Jan 23, 2025

robertbastian commented Jan 23, 2025

Choose a reason for hiding this comment

sffc commented Jan 23, 2025 • edited Loading

sffc commented Jan 24, 2025

sffc commented Jan 23, 2025 •

edited

Loading