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

Ergonomic compiled data APIs for property types #6049

Open
Manishearth opened this issue Jan 31, 2025 · 6 comments
Open

Ergonomic compiled data APIs for property types #6049

Manishearth opened this issue Jan 31, 2025 · 6 comments
Assignees
Labels
C-unicode Component: Props, sets, tries needs-approval One or more stakeholders need to approve proposal U-android-bionic Bionic in Android

Comments

@Manishearth
Copy link
Member

Manishearth commented Jan 31, 2025

See discussion #6048

We have a bunch of APIs for properties that go indirectly through CodePointSetData/etc. It would make sense from a convenience and discoverability point of view to have these also live on the property types. It's certainly how regular Rust users will expect this crate to work. I also think putting data loading front and center is far less important for most "singleton" APIs.

Concrete proposal. We add the following APIs to all types in icu_provider::props, duplicated on the

  • PropertyEnum::for_char(ch) / PropertyEnum::for_u32(ch)
  • PropertyEnum::long_name()/PropertyEnum::short_name()
  • Also a similar set of APIs for BinaryPropertyUnitStruct? Maybe?
  • PropertyEnum::try_from_str_strict(&str) OR PropertyEnum::try_from_str_loose(&str) (I'd like to just add one, not both, if possible. Perhaps we should pick loose?)

@sffc seems to be on board provided that we do not add FromStr/etc impls and clearly document that these are compiled_data only.

This might be necessary for ICU4X in Bionic: the current APIs are inefficient over FFI (see #6050 for the broader issue)

@robertbastian @zbraniecki thoughts?

@Manishearth Manishearth added C-unicode Component: Props, sets, tries U-android-bionic Bionic in Android labels Jan 31, 2025
@Manishearth Manishearth self-assigned this Jan 31, 2025
@Manishearth Manishearth added the needs-approval One or more stakeholders need to approve proposal label Jan 31, 2025
@Manishearth
Copy link
Member Author

Approval requested from

@sffc
Copy link
Member

sffc commented Jan 31, 2025

PropertyEnum::parse_strict(&str) should be try_from_str_strict

We should have name getters for both the property and the property value, yes?

@Manishearth
Copy link
Member Author

Fine with try_from_str_strict.

We should have name getters for both the property and the property value, yes?

The property name? Like General_Category? I don't think we have a parser for that right now, because we don't have runtime prop based APIs. The closest analogue are the new_for_ecma262 constructors, which do that parsing internally.

@sffc
Copy link
Member

sffc commented Jan 31, 2025

We should have name getters for both the property and the property value, yes?

The property name? Like General_Category? I don't think we have a parser for that right now, because we don't have runtime prop based APIs. The closest analogue are the new_for_ecma262 constructors, which do that parsing internally.

Yeah. If we don't have it now, we should plan for it in the function names. It's probably fine to have it code-driven instead of data-driven, since the set of properties we support is code-driven.

@Manishearth
Copy link
Member Author

Yeah. If we don't have it now, we should plan for it in the function names

I imagine if we ever get this it will be on a new enum. We actually had such an enum in my initial design of the enum props but we moved away from it. I think the current proposed names are compatible with this.

@Manishearth
Copy link
Member Author

Discussed

  • @robertbastian: Main concern: wish to add a constant number of names, not like get_foo_name, get_bar_name, etc

Proposal:

  • BidiClass::Blah.long_name(), .short_name()
  • BidiClass::for('a').
  • BidiClass::try_from_str(&str), does the loose thing. Use PropertyParser if you want strict. We can add try_from_str_strict over FFI if FFI users ask for it.
  • All of these come form the EnumProperty trait
  • We do the same thing on the BinaryPropertyTrait with for() -> bool
  • We do the exact same thing over FFI by adding singletons.
  • We document for() as not handling surrogates. FFI for() DOES handle surrogates. Document that distinction.
  • Names potentially to be bikeshed later. But there seems to be general agreement here on names.
  • Consider adding extension trait to char for 'a'.enum_property::<BidiClass>() later, not 2.0

LGTM: @sffc @Manishearth @echeran @robertbastian

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-unicode Component: Props, sets, tries needs-approval One or more stakeholders need to approve proposal U-android-bionic Bionic in Android
Projects
None yet
Development

No branches or pull requests

2 participants