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

Create TypedNeoDateTimeFormatter and DateTimePattern types #4415

Merged
merged 67 commits into from
Dec 30, 2023

Conversation

sffc
Copy link
Member

@sffc sffc commented Dec 6, 2023

Part of #3347
Depends on #4496
Depends on #4497

@sffc sffc force-pushed the neo3 branch 2 times, most recently from e9d20ba to 52a0ab1 Compare December 9, 2023 00:06
@sffc
Copy link
Member Author

sffc commented Dec 9, 2023

Background: the current DateTimeFormatter is over 6000 bytes. I've demonstrated in this PR that NeoDateTimeFormatter is around 700 bytes, which is a huge improvement, but we want to see if we can get it even smaller.

@sffc sffc marked this pull request as ready for review December 27, 2023 03:50
@sffc
Copy link
Member Author

sffc commented Dec 27, 2023

Review tips:

It should be easier to review this all at once. Commit-by-commit may result in your reviewing code that I subsequently refactored or deleted.

Most of this PR is plumbing. I tried to pull off the interesting parts as I was writing it. There are two more PRs, #4496 and #4497, that are currently open with two small but slightly more interesting bits of code.

This PR adds the following public types:

  1. icu_datetime::neo_pattern::DateTimePattern, a public wrapper around icu_datetime::pattern::runtime::Pattern, with a more restricted API surface sufficient to be used in icu_datetime::TypedDateTimeNames (which is already landed)
  2. icu_datetime::neo::TypedNeoDateFormatter, which supports formatting with date lengths and uses all neo data
  3. icu_datetime::neo::TypedNeoTimeFormatter, which supports formatting with time lengths and uses all neo data
  4. icu_datetime::neo::TypedNeoDateTimeFormatter, which supports formatting with date and time lengths and uses all neo data

The code in src/neo.rs and src/neo_pattern.rs is intended to be as thin as reasonably possible, with all business logic living in other modules such as the new src/raw/neo.rs. The code in src/neo.rs is mostly data provider marshaling; by the time you get into src/raw/neo.rs the data provider types are normalized for you.

I introduced a new "external loader" design pattern in src/external_loaders.rs in order to plumb into the correct constructors of types in dependency crates, such as FixedDecimalFormatter and WeekCalculator. I think I managed to get this to be fairly clean and I even have a macro to generate some of the boilerplate.

This implements part of the fix for #4340, but I didn't fix the whole thing in an effort to not scope creep this PR.

Copy link

dpulls bot commented Dec 27, 2023

🎉 All dependencies have been resolved !

@sffc sffc removed the request for review from nordzilla December 27, 2023 18:04
Manishearth
Manishearth previously approved these changes Dec 29, 2023
#[allow(clippy::too_many_arguments)]
pub(crate) fn load_for_pattern<YearMarker, MonthMarker>(
&mut self,
year_provider: Option<&(impl DataProvider<YearMarker> + ?Sized)>,
Copy link
Member

Choose a reason for hiding this comment

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

praise: good on keeping these traits only in the bounds of a single method rather than having them spread everywhere (and causing monomorphized code size issues)

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, with AnyCalendar <YearMarker, MonthMarker> are not known until runtime and I don't really want to include more than 1 copy of this function (i.e. through an enum switch). Maybe we can move those over to the external loader architecture so that the enum switch is isolated to the load function. Let's figure that out when I implement the AnyCalendar version of neo formatter.

Copy link
Member

Choose a reason for hiding this comment

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

Yep.

/// Trait for loading a FixedDecimalFormatter.
///
/// Implemented on the provider-specific loader types in this module.
pub(crate) trait FixedDecimalFormatterLoader {
Copy link
Member

Choose a reason for hiding this comment

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

observation: must stay private, we may need to be able to load RBNF in the long run

in the medium term we will probably hardcode a FixedDecimalFormatter | CustomFormat where the CustomFormat is a simple enum with code attached.

}

impl DateTimePattern {
/// Creates a [`DateTimePattern`] from a pattern string.
Copy link
Member

Choose a reason for hiding this comment

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

nit (nb, followup): roughly document thr format or link to UTS 35.

@sffc sffc changed the title Create DateTimePattern struct with data loading capabilities Create TypedNeoDateTimeFormatter and DateTimePattern types Dec 30, 2023
@sffc sffc merged commit ecc44ec into unicode-org:main Dec 30, 2023
29 checks passed
@sffc sffc deleted the neo3 branch December 30, 2023 07:47
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.

2 participants