Create TimeZoneFormatConfig enum #1256

samchen61661 · 2021-11-02T20:31:59Z

Create TimeZoneFormatConfig enum for #622

nordzilla · 2021-11-02T20:43:54Z

This looks great so far, and just the kind of enum we want for this.

As part of this same PR, we will want to also create a new constructor for TimeZoneFormat that accepts the enum as an argument.

For now I'm going to convert this PR to a draft PR and remove the other reviewers.

nordzilla · 2021-11-08T23:34:13Z

@samchen61661

So there are still a couple things left to do for this PR.

We need to go through and make parts of the API public that should be, and also remove the TODOs associated with the documentation comments. Try grepping for TODO and 622

src/zoned_datetime.rs:27:// TODO(#622) link [`TimeZoneFormat`] once it is public.
src/lib.rs:91:// TODO(#622) make the time_zone module public once TimeZoneFormat is public.
src/lib.rs:103:// TODO(#622) re-export TimeZoneFormat once it is made public.
src/format/time_zone.rs:44:// TODO(#622) Make this public once TimeZoneFormat is public.
src/mock/time_zone.rs:11:// TODO(#622) link [`TimeZoneFormat`] appropriately once it is public.
src/time_zone.rs:67:/// // TODO(#622) Uncomment and update example once TimeZoneFormat is public.
src/time_zone.rs:92:// TODO(#622) Make TimeZoneFormat public once we have a clean way to provide it options.
src/time_zone.rs:121:    /// // TODO(#622) Uncomment and update example once TimeZoneFormat is public.
src/time_zone.rs:137:    // TODO(#622) Make this public once TimeZoneFormat is public.
src/time_zone.rs:277:    /// // TODO(#622) Uncomment and update example once TimeZoneFormat is public.
src/time_zone.rs:302:    // TODO(#622) Make this public once TimeZoneFormat is public.
src/time_zone.rs:320:    /// // TODO(#622) Uncomment and update example once TimeZoneFormat is public.
src/time_zone.rs:349:    // TODO(#622) Make this public once TimeZoneFormat is public.
src/time_zone.rs:364:    /// // TODO(#622) Uncomment and update example once TimeZoneFormat is public.
src/time_zone.rs:389:    // TODO(#622) Make this public once TimeZoneFormat is public.

We should explore if we can remove the TimeZoneFormat::try_new() that takes a pattern, and see if we can logically replace it with the new public constructor that takes a pattern.

samchen61661 · 2021-11-09T21:24:39Z

@samchen61661

So there are still a couple things left to do for this PR.

We need to go through and make parts of the API public that should be, and also remove the TODOs associated with the documentation comments. Try grepping for TODO and 622

src/zoned_datetime.rs:27:// TODO(#622) link [`TimeZoneFormat`] once it is public.
src/lib.rs:91:// TODO(#622) make the time_zone module public once TimeZoneFormat is public.
src/lib.rs:103:// TODO(#622) re-export TimeZoneFormat once it is made public.
src/format/time_zone.rs:44:// TODO(#622) Make this public once TimeZoneFormat is public.
src/mock/time_zone.rs:11:// TODO(#622) link [`TimeZoneFormat`] appropriately once it is public.
src/time_zone.rs:67:/// // TODO(#622) Uncomment and update example once TimeZoneFormat is public.
src/time_zone.rs:92:// TODO(#622) Make TimeZoneFormat public once we have a clean way to provide it options.
src/time_zone.rs:121:    /// // TODO(#622) Uncomment and update example once TimeZoneFormat is public.
src/time_zone.rs:137:    // TODO(#622) Make this public once TimeZoneFormat is public.
src/time_zone.rs:277:    /// // TODO(#622) Uncomment and update example once TimeZoneFormat is public.
src/time_zone.rs:302:    // TODO(#622) Make this public once TimeZoneFormat is public.
src/time_zone.rs:320:    /// // TODO(#622) Uncomment and update example once TimeZoneFormat is public.
src/time_zone.rs:349:    // TODO(#622) Make this public once TimeZoneFormat is public.
src/time_zone.rs:364:    /// // TODO(#622) Uncomment and update example once TimeZoneFormat is public.
src/time_zone.rs:389:    // TODO(#622) Make this public once TimeZoneFormat is public.

We should explore if we can remove the TimeZoneFormat::try_new() that takes a pattern, and see if we can logically replace it with the new public constructor that takes a pattern.

Sure, I will do both in this PR.

But I have some questions:

re: 'We need to go through and make parts of the API public that should be':
It confuses me because TimeZoneFormat and TimeZoneFormatConfig are marked as public. Are there any things we need to do?
I need to have 1 to 1 match between pattern and config. And here are the gaps I found:

Do we treat ZZZZ the same as OOOO? They both long localized GMT format, but ZZZZ is GMT-8:00 while OOOO is GMT-08:00.
We talked about Vs during last meeting. And I found pattern supports VVV and VVV. Do we want to support them for config? If so, we need to add new enums for them.

It is more about implementation details. iso8601_format in TimeZoneFormat takes IsoFormat instead of &IsoFormat. I wonder why it is designed to take the value instead of the reference? I think I still want to own IsoFormat in the TimeZoneFormatConfig,

nordzilla · 2021-11-09T21:54:35Z

But I have some questions:

1. re: 'We need to go through and make parts of the API public that should be':
   It confuses me because `TimeZoneFormat` and `TimeZoneFormatConfig` are marked as public. Are there any things we need to do?

The goal of this PR is to make TimeZoneFormat fully pub (it is currently only pub(super), which means it is only public to the parent module), and provide a new, pub TimeZoneFormatConfig enum with a pub constructor that utilizes the enum, along with making the other format functions public (marked with TODO(#622)).

2. I need to have 1 to 1 match between pattern and config. And here are the gaps I found:


* Do we treat `ZZZZ` the same as `OOOO`? They both long localized GMT format, but `ZZZZ` is `GMT-8:00` while `OOOO` is `GMT-08:00`.

We are not differentiating between ZZZZ or OOOO in ICU4X. I'm sorry this is not documented more clearly. I will try to improve the clarity of the documentation here.

The reason that we only have one function called localized_gmt_format instead of something like long_gmt_format and short_gmt_format etc. is because we are choosing to always use the localized form of hourFormat as specified in the CLDR-JSON data:

https://github.com/unicode-org/cldr-json/blob/83f6144d8aa3d10e932c25924d4c903925b24257/cldr-json/cldr-dates-full/main/ar/timeZoneNames.json#L12

Rather than let developers arbitrarily choose between long or short GMT format, it seems like a better decision to always use the localized form that is specified by the locale. So in ICU4X, both ZZZZ and OOOO use the same localized GMT format which refers to the locale's specified hourFormat entry.

* We talked about `V`s during last meeting. And I found pattern supports `VVV` and `VVV`. Do we want to support them for config? If so, we need to add new enums for them.

I went back and revisited my reasoning for not including ExemplarCity in TimeZoneFormatConfig as a stand-alone format. It's because we offer GenericLocation, // e.g. Los Angeles Time which contains the exemplar city in addition to the localized word(s) for Time. This seems like a better option for developers than only the exemplar city by itself. And again, I would prefer to start with fewer options and add them as needed, rather than start with many options and remove them.

1. It is more about implementation details. `iso8601_format` in `TimeZoneFormat` takes `IsoFormat` instead of `&IsoFormat`. I wonder why it is designed to take the value instead of the reference? I think I still want to own `IsoFormat` in the `TimeZoneFormatConfig`,

I appreciate you calling this out. I thought that I had made IsoFormat a Copy type, but apparently I did not. My opinion is that iso8601_format should still take IsoFormat by value, but we should add #[derive(Clone, Copy)] above the declaration for the IsoFormat enum type.

samchen61661 · 2021-11-09T23:08:50Z

Thanks Eric!

I took a closer look at the code again. I found that TimeZoneFormat is dependent on DateTimeFormat. For example, here is a snippet of code:

        let datetime_format = DateTimeFormat::new(locale, patterns, symbols_data, ordinal_rules);
        let time_zone_format = TimeZoneFormat::try_new(
            datetime_format.locale.clone(),
            datetime_format
                // Only dates have plural variants so we can use any of the patterns for the time segment.
                .patterns
                .clone(),
            zone_provider,
        )?;

It feels like I need to create a mapping from pattern in DateTimeFormat to config in TimeZoneFormat to get rid of the dependency. But because we want to get rid of patterns, I think that's not an ideal solution.

nordzilla · 2021-11-09T23:16:24Z

Thanks Eric!

I took a closer look at the code again. I found that TimeZoneFormat is dependent on DateTimeFormat. For example, here is a snippet of code:
        let datetime_format = DateTimeFormat::new(locale, patterns, symbols_data, ordinal_rules);
        let time_zone_format = TimeZoneFormat::try_new(
            datetime_format.locale.clone(),
            datetime_format
                // Only dates have plural variants so we can use any of the patterns for the time segment.
                .patterns
                .clone(),
            zone_provider,
        )?;
It feels like I need to create a mapping from pattern in DateTimeFormat to config in TimeZoneFormat to get rid of the dependency. But because we want to get rid of patterns, I think that's not an ideal solution.

@samchen61661

For now, this is why we're creating two constructors: the pre-existing try_new() and our new constructor that takes a config object.

For this PR, it would be fine to keep the try_new() constructor as pub(super), which gets used only by ZonedDateTimeFormat and only publicly expose our new constructor.

It would be great if we could find a way to consolidate and eventually remove try_new(), finding a way that ZoneDateTimeFormat could have a mapping from its patterns to the config items.

This is what I meant when I said:

We should explore if we can remove the TimeZoneFormat::try_new() that takes a pattern, and see if we can logically replace it with the new public constructor that takes a pattern.

But after thinking about it further, since we're not including the VVV pattern in the public API, it won't be easy to consolidate this. But we can always look at this further in a follow-up PR.

For now, I think the best option would be to keep try_new() as pub(super) and just remove its TODO comment, only exposing our new constructor as fully pub.

samchen61661 · 2021-11-10T00:24:16Z

Thanks Eric!
I took a closer look at the code again. I found that TimeZoneFormat is dependent on DateTimeFormat. For example, here is a snippet of code:
        let datetime_format = DateTimeFormat::new(locale, patterns, symbols_data, ordinal_rules);
        let time_zone_format = TimeZoneFormat::try_new(
            datetime_format.locale.clone(),
            datetime_format
                // Only dates have plural variants so we can use any of the patterns for the time segment.
                .patterns
                .clone(),
            zone_provider,
        )?;
It feels like I need to create a mapping from pattern in DateTimeFormat to config in TimeZoneFormat to get rid of the dependency. But because we want to get rid of patterns, I think that's not an ideal solution.
@samchen61661

For now, this is why we're creating two constructors: the pre-existing try_new() and our new constructor that takes a config object.

For this PR, it would be fine to keep the try_new() constructor as pub(super), which gets used only by ZonedDateTimeFormat and only publicly expose our new constructor.

It would be great if we could find a way to consolidate and eventually remove try_new(), finding a way that ZoneDateTimeFormat could have a mapping from its patterns to the config items.

This is what I meant when I said:

We should explore if we can remove the TimeZoneFormat::try_new() that takes a pattern, and see if we can logically replace it with the new public constructor that takes a pattern.

But after thinking about it further, since we're not including the VVV pattern in the public API, it won't be easy to consolidate this. But we can always look at this further in a follow-up PR.

For now, I think the best option would be to keep try_new() as pub(super) and just remove its TODO comment, only exposing our new constructor as fully pub.

Sounds good!

nordzilla

Looking great so far!

There are just a couple changes to make in the review comments, and we also need to add tests for this new functionality before we can land this.

I'm happy to sync up on a call about testing and walk you through how our test suites work in more detail if that would be helpful.

components/datetime/src/format/time_zone.rs

components/datetime/src/time_zone.rs

nordzilla

Looks really great @samchen61661!

Just a few minor changes, and I'll also add Shane back on as a reviewer.

components/datetime/src/format/time_zone.rs

components/datetime/src/time_zone.rs

components/datetime/tests/datetime.rs

nordzilla

Just a couple small things I missed on the first pass, otherwise this looks really great to me!

I'll leave it to Shane to lend a second pair of eyes before we land this.

Really excited to make this public!

components/datetime/src/format/time_zone.rs

components/datetime/src/time_zone.rs

samchen61661 · 2021-11-12T04:55:24Z

Just a couple small things I missed on the first pass, otherwise this looks really great to me!

I'll leave it to Shane to lend a second pair of eyes before we land this.

Really excited to make this public!

Thanks for your guidance Eric! I learnt a lot from you these days! I did not expect that it touched so many files eventually and I felt that I am more comfortable to read and write Rust code. And I am very happy to make TimeZoneConfig public!

nordzilla

Thanks so much for working on this. This looks good to me.

Technically it's missing some documentation comments, such as on the newly added function TimeZoneFormat::try_from_config(), but once this lands I'm planning to go through and read all the docs/update them/add anything that's missing, so I'm comfortable landing this as is with docs as a follow-up.

sffc

Looks good overall. I found some small areas for improvement. We'll probably need to add more configs to this enum in the future, but now we have a straightforward place to add them.

For example:
https://tc39.es/proposal-intl-extend-timezonename/#sec-basicformatmatcher

components/datetime/src/format/time_zone.rs

components/datetime/src/time_zone.rs

nordzilla

This looks good. Just one small nitpick from me.

Good call on removing those expect calls @sffc.

components/datetime/src/format/time_zone.rs

nordzilla · 2021-11-17T00:13:32Z

components/datetime/src/format/time_zone.rs

+        }
+        (None, None) => unreachable!(
+            "TimeZoneFormat should have either a configuration or a pattern, but found neither"
+        ),


@sffc

By your same logic about the expect potentially adding unwanted panic machinery, should we remove the unreachable!() calls here and return an error instead?

I'm not quite as concerned about a plain unreachable!() because it's more self-contained; .expect() pulls in the formatting machinery since it prints a formatted message*. If we build with panic_abort, then any unreachable!() should basically compile away.

That being said, if you want to avoid the unreachable code path, you can use an internal exhaustive enum such as

enum TimeZoneFormatKind { Config(TimeZoneFormatConfig), Pattern(DataPayload<'data, PatternPluralsFromPatternsV1Marker>), }

This would likely be a good cleanup to your code regardless.

* It actually appears that the fmt::Debug bound gets pulled in for Result but maybe not Option

I'm re-opening this thread because I think switching to use an enum may be a beneficial change for @samchen61661 to consider.

Yeah, it is a good suggestion. I am not familiar with the open source policy. What do you think to improve it in a separate PR?

The main concern here is that this PR is big and the fix is relatively small.

Got it.

When you said "This would likely be a good cleanup to your code regardless." I thought you meant as a follow-up.

Happy to add it in here to as I agree that it is a good change.

@samchen61661 you can go ahead and add it to this PR.

I'm fine landing this as-is and doing this refactor in a small follow-up PR. But since you still need to resolve the merge conflict in this PR, it seems to make sense to fix this while you're at it.

I see. sg and I will refactor it in this PR.

Shane, when you talked about you can use an INTERNAL exhaustive enum, are you saying that I just need to use TimeZoneFormatKind in the write_zone method?

I think we can make TimeZoneFormatKind as an external attribute in TimeZoneFormat to replace config and patterns.

@samchen61661

What we would need to do is:

pub(super) TimeZoneFormatKind<'data> { ... }

The enum should only be internal (i.e. pub(super)) because users of the public API shouldn't have to worry about this type.

And then instead of having data members for config and patterns, you would just have one data member kind that is a TimeZoneFormatKind.

pub struct TimeZoneFormat<'data> { + pub(super) kind: TimeZoneFormatKind<'data>, - pub(super) config: ... - pub(super) patterns: ... ... }

Note that the enum TimeZoneFormatKind will need to have a lifetime specifier since it holds the DataPayload for the pattern variant.

components/datetime/src/time_zone.rs

components/datetime/src/format/time_zone.rs

components/datetime/src/time_zone.rs

components/datetime/src/format/time_zone.rs

nordzilla

This looks good to me, just pending my one question to Shane regarding the panic handling:
#1256 (comment)

components/datetime/src/time_zone.rs

nordzilla

Looks good to me!

Thanks for being willing to make all of these small changes over several iterations.

I think the code is overall cleaner because of it.

jira-pull-request-webhook · 2021-11-23T21:25:21Z

Notice: the branch changed across the force-push!

components/datetime/src/format/time_zone.rs is different
components/datetime/src/time_zone.rs is different
components/datetime/src/zoned_datetime.rs is different
components/datetime/tests/datetime.rs is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

samchen61661 requested review from gregtatum, nordzilla and zbraniecki as code owners November 2, 2021 20:32

nordzilla linked an issue Nov 2, 2021 that may be closed by this pull request

Create a config enum for initializing TimeZoneFormat #622

Closed

nordzilla removed request for zbraniecki and gregtatum November 2, 2021 20:44

nordzilla marked this pull request as draft November 2, 2021 20:44

samchen61661 marked this pull request as ready for review November 10, 2021 00:26

nordzilla requested changes Nov 10, 2021

View reviewed changes

components/datetime/src/format/time_zone.rs Outdated Show resolved Hide resolved

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

components/datetime/src/time_zone.rs Show resolved Hide resolved

nordzilla requested changes Nov 12, 2021

View reviewed changes

nordzilla requested a review from sffc November 12, 2021 00:46

nordzilla reviewed Nov 12, 2021

View reviewed changes

components/datetime/tests/datetime.rs Outdated Show resolved Hide resolved

nordzilla requested changes Nov 12, 2021

View reviewed changes

components/datetime/src/format/time_zone.rs Outdated Show resolved Hide resolved

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

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

nordzilla previously approved these changes Nov 12, 2021

View reviewed changes

sffc requested changes Nov 13, 2021

View reviewed changes

samchen61661 dismissed nordzilla’s stale review via 686d192 November 15, 2021 23:20

nordzilla self-requested a review November 17, 2021 00:08

nordzilla requested changes Nov 17, 2021

View reviewed changes

samchen61661 commented Nov 17, 2021

View reviewed changes

nordzilla mentioned this pull request Nov 17, 2021

Retrieve localized metazone info from timeZoneNames.json in TimeZonesProvider #993

Closed

nordzilla self-requested a review November 18, 2021 16:59

nordzilla previously approved these changes Nov 18, 2021

View reviewed changes

nordzilla requested a review from sffc November 18, 2021 21:15

sffc previously approved these changes Nov 22, 2021

View reviewed changes

components/datetime/src/time_zone.rs Show resolved Hide resolved

samchen61661 dismissed stale reviews from sffc and nordzilla via ccae740 November 22, 2021 19:58

nordzilla previously approved these changes Nov 22, 2021

View reviewed changes

sffc previously approved these changes Nov 22, 2021

View reviewed changes

samchen61661 dismissed stale reviews from sffc and nordzilla via e6a8b3b November 23, 2021 20:23

samchen61661 added 6 commits November 23, 2021 12:25

Create TimeZoneFormatConfig enum

3e7ba85

rebase master

fe8d8dc

rebase main again

55dcf05

rebase onto 5c15123

68ccddd

rebase onto d69131f

5727a15

add tests

726734b

nordzilla previously approved these changes Nov 23, 2021

View reviewed changes

samchen61661 added 8 commits November 23, 2021 13:04

remove clone and unwrap methods

03fc30a

address minor comments

3d8e136

add enums in struct file

911fa4c

uncomment examples

e3fd533

remove except method

8eb4b12

pass config by value

10ee8c5

create enum TimeZoneFormatKind

7a7890f

fix rebase error

aba20ed

samchen61661 dismissed nordzilla’s stale review via aba20ed November 23, 2021 21:25

samchen61661 force-pushed the issue-622 branch from e6a8b3b to aba20ed Compare November 23, 2021 21:25

sffc approved these changes Nov 23, 2021

View reviewed changes

nordzilla merged commit 6edc9d4 into unicode-org:main Nov 24, 2021

samchen61661 deleted the issue-622 branch April 6, 2022 01:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create TimeZoneFormatConfig enum #1256

Create TimeZoneFormatConfig enum #1256

samchen61661 commented Nov 2, 2021

nordzilla commented Nov 2, 2021

nordzilla commented Nov 8, 2021

samchen61661 commented Nov 9, 2021

nordzilla commented Nov 9, 2021 •

edited

samchen61661 commented Nov 9, 2021

nordzilla commented Nov 9, 2021 •

edited

samchen61661 commented Nov 10, 2021

nordzilla left a comment

nordzilla left a comment

nordzilla left a comment

samchen61661 commented Nov 12, 2021

nordzilla left a comment

sffc left a comment

nordzilla left a comment

nordzilla Nov 17, 2021

sffc Nov 22, 2021 •

edited

sffc Nov 22, 2021

samchen61661 Nov 22, 2021

nordzilla Nov 22, 2021

sffc Nov 22, 2021

samchen61661 Nov 22, 2021

samchen61661 Nov 22, 2021

nordzilla Nov 22, 2021 •

edited

nordzilla left a comment

nordzilla left a comment

jira-pull-request-webhook bot commented Nov 23, 2021

Create TimeZoneFormatConfig enum #1256

Create TimeZoneFormatConfig enum #1256

Conversation

samchen61661 commented Nov 2, 2021

nordzilla commented Nov 2, 2021

nordzilla commented Nov 8, 2021

samchen61661 commented Nov 9, 2021

nordzilla commented Nov 9, 2021 • edited

samchen61661 commented Nov 9, 2021

nordzilla commented Nov 9, 2021 • edited

samchen61661 commented Nov 10, 2021

nordzilla left a comment

Choose a reason for hiding this comment

nordzilla left a comment

Choose a reason for hiding this comment

nordzilla left a comment

Choose a reason for hiding this comment

samchen61661 commented Nov 12, 2021

nordzilla left a comment

Choose a reason for hiding this comment

sffc left a comment

Choose a reason for hiding this comment

nordzilla left a comment

Choose a reason for hiding this comment

nordzilla Nov 17, 2021

Choose a reason for hiding this comment

sffc Nov 22, 2021 • edited

Choose a reason for hiding this comment

sffc Nov 22, 2021

Choose a reason for hiding this comment

samchen61661 Nov 22, 2021

Choose a reason for hiding this comment

nordzilla Nov 22, 2021

Choose a reason for hiding this comment

sffc Nov 22, 2021

Choose a reason for hiding this comment

samchen61661 Nov 22, 2021

Choose a reason for hiding this comment

samchen61661 Nov 22, 2021

Choose a reason for hiding this comment

nordzilla Nov 22, 2021 • edited

Choose a reason for hiding this comment

nordzilla left a comment

Choose a reason for hiding this comment

nordzilla left a comment

Choose a reason for hiding this comment

jira-pull-request-webhook bot commented Nov 23, 2021

nordzilla commented Nov 9, 2021 •

edited

nordzilla commented Nov 9, 2021 •

edited

sffc Nov 22, 2021 •

edited

nordzilla Nov 22, 2021 •

edited