Create BoundProvider, NeoFormatter, and TypedNeoFormatter #4877

sffc · 2024-05-08T06:55:54Z

Part of #1317

This PR adds TypedNeoFormatter and NeoFormatter that use a type parameter to handle all datetime formatting.

This PR is productive because:

The reduced number of APIs reduces the total amount of work we need to do to get to the finish line
The resulting API will be more extensible and make easier to add additional skeleta of various shapes
The stack sizes for the formatters are smaller than they have ever been

Some notes:

Everything will be powered through skeleta. I will make datetime lengths into skeleta, too, and get rid of that data key and separate code path.
To signal a key that is not needed for a particular skeleton: NeverMarker is used for the DataProvider bound, and () is used for storage types.
I plan to delete all of the other neo formatters once this lands, maybe in the same PR
After I delete the other formatters, I should be able to remove some unreachables / failure cases and internal enum variants, which will be nice. Some will be easier to eliminate than others.
Not sure I'm completely happy having two separate type parameters in TypedNeoFormatter (one for the calendar and one for the skeleton); happy for suggestions.
This same model will be useful for adding the time zone formatter.

Still draft for now because I need to make anycalendar compile. I know how to do it, it's just annoying.

robertbastian

Some preliminary thoughts

robertbastian · 2024-05-08T09:08:19Z

components/datetime/src/format/neo.rs

+    /// The specific type does not support this field
+    TypeTooNarrow(Field),


It's not clear which "type" is too "narrow". I think this is the same as UnsupportedField, we should just update the doc string to say "The formatter doesn't support this field". I don't think we need to differentiate between ICU4X not supporting a field, and a selected formatter not supporting a field, ICU4X is just the union of all formatters, but this union could grow.

robertbastian · 2024-05-08T09:09:47Z

components/datetime/src/format/neo.rs

    _calendar: PhantomData<C>,
 }

+pub trait DateTimeNamesMarker {
+    type YearNames: MaybePayload<YearNamesV1<'static>> + fmt::Debug;


suggestion: put the Debug bound on MaybePayload.

robertbastian · 2024-05-08T09:11:25Z

components/datetime/src/format/neo.rs

+    year_symbols: OptionalNames<(), R::YearNames>,
+    month_symbols: OptionalNames<fields::Month, R::MonthNames>,
+    weekday_symbols: OptionalNames<fields::Weekday, R::WeekdayNames>,
+    dayperiod_symbols: OptionalNames<(), R::DayPeriodNames>,
    // TODO(#4340): Make the FixedDecimalFormatter optional
    fixed_decimal_formatter: Option<FixedDecimalFormatter>,
    week_calculator: Option<WeekCalculator>,


week_calculator is also not needed for time formatting, can we store the raw payload as a MaybePayload?

It's a little harder because it is cross-crate. Added a TODO.

robertbastian · 2024-05-08T09:22:12Z

components/datetime/src/neo.rs

+        .map_err(LoadError::Data)?;
+        let mut names = RawDateTimeNames::new_without_fixed_decimal_formatter();
+        names
+            .load_for_pattern::<<R::Data as TypedNeoSkeletonData<C>>::YearNamesV1Marker, <R::Data as TypedNeoSkeletonData<C>>::MonthNamesV1Marker, <R::Data as NeoSkeletonCommonData>::WeekdayNamesV1Marker, <R::Data as NeoSkeletonCommonData>::DayPeriodNamesV1Marker>(


Is a LoadError::MissingNames actually possible here?

This is narrowed down to only WeekCalculator and FixedDecimalFormatter now. I added a TODO to give those the same treatment I gave the others, and then we can eliminate this error variant.

(cherry picked from commit 729870d)

Manishearth

discussed design in meeting today, things made sense. would be nice to further document most of the traits here by their purpose and what they interact with (if not done already)

Manishearth · 2024-05-09T18:46:59Z

components/datetime/src/calendar.rs

@@ -81,86 +83,6 @@ pub trait CldrCalendar: InternalCldrCalendar {
    }
 }

-#[cfg(feature = "experimental")]
-pub(crate) trait YearNamesV1Provider<M: DataMarker> {


Manishearth · 2024-05-09T18:47:25Z

components/datetime/src/calendar.rs

+/// A collection of marker types associated with all calendars.
+#[allow(private_bounds)] // sealed
+#[cfg(any(feature = "datagen", feature = "experimental"))]
+pub trait CalMarkers<M>: Sealed


thought: this trait feels backwards to me but i also think this is the best way to do this.

suggestion: more docs maybe, covering how it gets used?

Manishearth · 2024-05-09T18:51:05Z

components/datetime/src/neo.rs

-            + DataProvider<RocDatePatternV1Marker>
-            + DataProvider<RocYearNamesV1Marker>
-            + DataProvider<RocMonthNamesV1Marker>
+            + DataProvider<<FullData as CalMarkers<YearNamesV1Marker>>::Buddhist>


thought: ew. but reasonable.

Manishearth · 2024-05-09T18:52:07Z

provider/core/src/data_provider.rs

+/// Unlike [`DataProvider`], the provider is bound to a specific key ahead of time.
+///
+/// [`AnyMarker`]: crate::any::AnyMarker
+pub trait BoundDataProvider<M>


(have expressed opinions about docs here on the other PR, fine to have this land as is and then do a followup)

sffc added 11 commits May 7, 2024 15:10

Add initial empty marker trait for DateTimeNames

6c73bc6

Working WeekdayNames assoc type

3efbbd1

Move for<'a> to the right place

24b30bd

Reveal the impact to neo_time_formatter_size

76d9bd4

DayPeriod treatment

14c443b

Year and Month treatment

ff377f7

TypeTooNarrow error

90d936e

Checkpoint: TypedNeoFormatter

420f865

TypedNeoFormatter is passing tests

ea246e8

Refactor / generalize NeoSkeleton traits to prepare for AnyCalendar

007ec24

Mostly compiling AnyCalendar formatter

08d7d7a

sffc requested review from Manishearth and robertbastian May 8, 2024 06:56

robertbastian reviewed May 8, 2024

View reviewed changes

sffc added 6 commits May 8, 2024 10:44

Add missing std pointer-like impls for DataProvider, DynamicDataProvider

95282ea

(cherry picked from commit 729870d)

Improving abstractions to collapse an error case

ccd9c17

Create BoundDataProvider

a5a9233

Switch to using BoundDataProvider

e4c2ca1

Migrate the other keys to BoundDataProvider

abb3ffa

Progress on AnyCalendarProvider

db8f0f9

sffc force-pushed the datetimenames-marker branch from 33bece5 to db8f0f9 Compare May 8, 2024 21:26

sffc added 8 commits May 8, 2024 14:48

AnyCalendarProvider4

970f861

Progress

ac49bd7

Progress

9bbe2bb

Phew, AnyCalendar skeleta NeoFormatter working

93ef6db

Consolidate traits

cc8bd95

Cleanup

f48c07f

Merge branch 'main' into datetimenames-marker

4da1935

Features

4c4d95c

sffc marked this pull request as ready for review May 9, 2024 17:11

sffc requested review from zbraniecki and nordzilla as code owners May 9, 2024 17:11

Manishearth previously approved these changes May 9, 2024

View reviewed changes

sffc added 4 commits May 9, 2024 12:15

Add TODOs

8af5d3c

Clippy, feedback

b3b16fd

Docs links

4ac6ec2

fmt

639c250

sffc dismissed Manishearth’s stale review via 639c250 May 9, 2024 19:18

sffc changed the title ~~Two datetime formatters to rule them all~~ Create BoundProvider, NeoFormatter, and TypedNeoFormatter May 9, 2024

sffc mentioned this pull request May 9, 2024

Create BoundDataProvider, a DataProvider that always loads for the same key #4883

Closed

sffc added 2 commits May 9, 2024 12:39

CHANGELOG

aa101c7

Add extra comment about use case for BoundDataProvider

2be79e3

sffc requested a review from a team as a code owner May 9, 2024 19:42

Manishearth approved these changes May 9, 2024

View reviewed changes

sffc merged commit 84986f7 into unicode-org:main May 9, 2024
30 checks passed

sffc deleted the datetimenames-marker branch May 9, 2024 20:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create BoundProvider, NeoFormatter, and TypedNeoFormatter #4877

Create BoundProvider, NeoFormatter, and TypedNeoFormatter #4877

sffc commented May 8, 2024

robertbastian left a comment

robertbastian May 8, 2024

robertbastian May 8, 2024

robertbastian May 8, 2024

sffc May 9, 2024

robertbastian May 8, 2024

sffc May 9, 2024

Manishearth left a comment

Manishearth May 9, 2024

Manishearth May 9, 2024

Manishearth May 9, 2024

Manishearth May 9, 2024

		/// The specific type does not support this field
		TypeTooNarrow(Field),

Create BoundProvider, NeoFormatter, and TypedNeoFormatter #4877

Create BoundProvider, NeoFormatter, and TypedNeoFormatter #4877

Conversation

sffc commented May 8, 2024

robertbastian left a 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

Manishearth left a 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