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

Bikeshed: What should neo datetime placeholder markers be called #4186

Closed
Manishearth opened this issue Oct 19, 2023 · 5 comments · Fixed by #4674
Closed

Bikeshed: What should neo datetime placeholder markers be called #4186

Manishearth opened this issue Oct 19, 2023 · 5 comments · Fixed by #4674
Assignees
Labels
A-data Area: Data coverage or quality C-datetime Component: datetime, calendars, time zones

Comments

@Manishearth
Copy link
Member

Manishearth commented Oct 19, 2023

Part of #3865

From #4185

We have an opportunity to reduce data size for calendars that don't need, say, day symbols data (or numeric month formatting data). For generic and AnyCalendar formatting to work, however, we need to be able to abstract over calendars that both have and don't have this data alike. Ideally we can do something like type DaySymbols = (), however this causes trouble when we attempt to abstract over this with traits since you can't easily do "or" bounds with traits (here we need "Yokeable = LinearSymbolsV1 OR Self = ()").

We should have an always-empty key for this, like PlaceholderDaySymbolsV1, which we never attempt to load but exists to satisfy the trait bound.

We should figure out what to call it.

(In general I'd like to avoid the term "dummy", it's often considered non-inclusive)

@Manishearth Manishearth added discuss Discuss at a future ICU4X-SC meeting A-data Area: Data coverage or quality labels Oct 19, 2023
@Manishearth Manishearth added this to the 1.4 Blocking ⟨P1⟩ milestone Oct 19, 2023
@sffc
Copy link
Member

sffc commented Nov 2, 2023

Discuss with:

@sffc sffc added the discuss-triaged The stakeholders for this issue have been identified and it can be discussed out-of-band label Nov 2, 2023
@Manishearth Manishearth moved this to Needs discussion to unblock in icu4x 2.0 Feb 23, 2024
@sffc sffc added the C-datetime Component: datetime, calendars, time zones label Feb 29, 2024
@Manishearth Manishearth changed the title Bikeshed: What should placeholder markers be called Bikeshed: What should neo datetime placeholder markers be called Feb 29, 2024
@Manishearth
Copy link
Member Author

Approach:

  • Have PlaceholderDaySymbolsV1 => LinearSymbolsV1 marker type, name TBD
  • In DTF data loading skip loading data when you encounter CldrCalendar assoc types that are placeholder. Either use type_id or something else.

Name bikeshed:

  • PlaceholderDaySymbolsV1
  • EmptyDaySymbolsV1
  • EmptyMarker<Yokeable>

Decision: EmptyMarker<Yokeable>

impl<Y: Yokeable> KeyedDataMarker for EmptyMarker<Y> {
    const KEY: DataKey = data_key!("_empty", singleton);
}

// on most providers
impl<Y: Yokeable> DataProvider<EmptyMarker<Y>> for Foo {
    debug_assert!(false);
    return DataError::MissingDataKey.with_key(EmptyMarker<Y>::KEY);
}


// Check for the EmptyMarker before calling .load()
// this should be a convenience method on EmptyMarker
if core::any::TypeId::of::<M>() == core::any::TypeId::of::<EmptyMarker<M::Yokeable>>() {
    // skip loading
}

Discussion:

  • rob: should not need a new error variant, since it's not something to catch and handle
  • manish: also want same behavior for blob.
  • manish: ultimately should never be called (and documented as such)
  • sffc: debug assert in AsDeserializingBufferProvider?
  • manish: yes

Agreed: @Manishearth @robertbastian @sffc

@Manishearth Manishearth removed discuss Discuss at a future ICU4X-SC meeting discuss-triaged The stakeholders for this issue have been identified and it can be discussed out-of-band labels Mar 6, 2024
@Manishearth Manishearth moved this from Needs discussion to unblock to Unclaimed for sprint in icu4x 2.0 Mar 6, 2024
@sffc
Copy link
Member

sffc commented Mar 11, 2024

I need this for skeleta, so I am taking it now

@sffc sffc self-assigned this Mar 11, 2024
@sffc
Copy link
Member

sffc commented Mar 11, 2024

Since the key fails to load, instead of EmptyMarker, wdyt of NeverMarker?

@Manishearth
Copy link
Member Author

works!

sffc added a commit that referenced this issue Mar 12, 2024
@github-project-automation github-project-automation bot moved this from Unclaimed for sprint to Done in icu4x 2.0 Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-data Area: Data coverage or quality C-datetime Component: datetime, calendars, time zones
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants