Add FFI for DateTimeFormat #2164
Conversation
ffi/diplomat/src/calendar.rs
Outdated
| pub struct ICU4XGregorianDateTime(pub DateTime<Gregorian>); | ||
|
|
||
| impl ICU4XGregorianDateTime { | ||
| /// Creates a new [`ICU4XGregorianDateTime`] from locale data. |
There was a problem hiding this comment.
nit: not from locale data
also I'd just call this new
| /// An ICU4X TimeFormat object capable of formatting a [`ICU4XGregorianDateTime`] as a string, | ||
| /// using the Gregorian Calendar. | ||
| #[diplomat::rust_link(icu::datetime::TimeFormat, Struct)] | ||
| pub struct ICU4XGregorianTimeFormat(pub TimeFormat<Gregorian>); |
There was a problem hiding this comment.
nit: pop in a todo and a link to #2153 since we probably(?) want this to just be TimeFormat eventually
ffi/diplomat/src/datetime.rs
Outdated
|
|
||
| /// Formats a [`ICU4XGregorianDateTime`] to a string. | ||
| #[diplomat::rust_link(icu::datetime::TimeFormat::format_to_write, FnInStruct)] | ||
| pub fn format_to_write( |
There was a problem hiding this comment.
thought: should this be format_datetime_to_write()? Would we end up adding a Time type over FFI?
There was a problem hiding this comment.
issue: Also, over FFI Writeables typically just return strings, I'd just call this format() (or, assuming the previous suggestion, format_datetime(). The three-way distinction on the Rust side is irrelevant here
|
I believe the CI failures here are due to rust-lang/rustup#3029 |
|
Going to merge based on @Manishearth's approval and because I would rather @zbraniecki than @dminor fix the naming to Formatter instead of Format |
Fixes #1326