-
Notifications
You must be signed in to change notification settings - Fork 182
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
Add initial neo datetime FFI #6035
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue: please replace the existing API instead of adding an additional API. Any Neo
name that you introduce now will just need to be cleaned up next week, and any code you don't delete today will have to be maintained until someone gets around to deleting it (we were in this state in the datetime crate for too long, it sucked).
Also, I would like to review a diff, not a brand new API that I manually have to compare to the old one. This applies to both the Diplomat definitions, as well as the C++ and Dart tests.
I think the point here is to migrate one API, and then if that goes well migrate the rest with the same pattern, but the APIs are interconnected so it's not possible to do that without "Neo". I guess an alternate route would be to have this PR as a draft to show the new API, get it reviewed but not landed, and then do the whole migration. |
Like in Rust, the new FFI API is a different mental model than the old FFI API, and it is not based on the old one. Consider it a new addition to icu_capi. I plan to delete (rather than migrate) the old one, which I am happy to do in the same PR. I am asking you to review the shape of the new API to catch any architectural issues before I spend time copying it into all the rest of the types. |
I marked it as draft to indicate that I don't plan to land this in its current form, but I would still like a review of it. |
Old C++: std::unique_ptr<DateTimeFormatter> any_dtf = DateTimeFormatter::create_with_length(
*locale.get(),
DateTimeLength::Medium
).ok().value();
out = any_dtf->format(*any_date.get(), *any_time.get()).ok().value();
std::cout << "Formatted value is " << out << std::endl;
if (out != "Oct 5, 2 Reiwa, 1:33\u202fPM") {
std::cout << "Output does not match expected output" << std::endl;
return 1;
} New C++, static field set: std::unique_ptr<NeoDateTimeFormatter> fmt_ymdt = NeoDateTimeFormatter::create_ymdt(
*locale.get(),
NeoDateTimeLength::Medium,
TimePrecision::Minute,
DateTimeAlignment::Auto,
YearStyle::Auto
).ok().value();
out = fmt_ymdt->format_iso(*date.get(), *time.get());
std::cout << "Fieldset YMDT: " << out;
if (out != "11 jul 2022, 13:06") {
std::cout << " (unexpected!)";
saw_unexpected_output = true;
}
std::cout << std::endl; New C++, dynamic field set: DateTimeFieldSetBuilder builder;
builder.length = std::optional<NeoDateTimeLength>(NeoDateTimeLength::Long);
builder.date_fields = std::optional<DateFields>(DateFields::YM);
std::unique_ptr<NeoDateTimeFormatter> fmt_ym_bld = NeoDateTimeFormatter::create_from_builder(
*locale.get(),
builder
).ok().value();
out = fmt_ym_bld->format_iso(*date.get(), *time.get());
std::cout << "Fieldset YM in DateTimeFormatter via builder: " << out;
if (out != "julio de 2022") {
std::cout << " (unexpected!)";
saw_unexpected_output = true;
}
std::cout << std::endl; (these examples are not doing the exact same thing but they are all end-to-end formatting call sites) |
Some specific things I want comments on: Field sets are modeled as constructors with positional arguments. For example, in Rust there is a struct with three fields: https://unicode-org.github.io/icu4x/rustdoc/icu/datetime/fieldsets/struct.MDT.html In C++, I modeled it as a constructor with the same three positional arguments. I do not currently have a discriminator in the constructor name as we do in other places. Should I add one? For example, if we add a new semantic option in the future (not unlikely), it would need to be added as a positional argument. We have the I could also use a struct. I would name the struct based on what it contains, not what it does, so that the same struct can be shared across multiple field set constructors. Or, I could use the builder struct in the field set constructors, and make the constructor fail at runtime if the builder struct is not compatible with the requested field set. For example: // Currently in PR:
// Infallible except for data-related errors:
NeoDateTimeFormatter::create_ymdt(
*locale.get(),
NeoDateTimeLength::Medium,
TimePrecision::Minute,
DateTimeAlignment::Auto,
YearStyle::Auto
)
// Alternative A:
// Infallible except for data-related errors:
NeoDateTimeFormatter::create_ymdt_with_length_time_precision_alignment_and_year_style(
*locale.get(),
NeoDateTimeLength::Medium,
TimePrecision::Minute,
DateTimeAlignment::Auto,
YearStyle::Auto
)
// Alternative B:
LengthTimePrecisionAlignmentYearStyleOptions options;
options.length = std::optional<NeoDateTimeLength>(NeoDateTimeLength::Medium);
options.time_precision = std::optional<TimePrecision>(TimePrecision::Minute);
options.alignment = std::optional<DateTimeAlignment>(DateTimeAlignment::Auto);
options.year_style = std::optional<YearStyle>(YearStyle::Auto);
// Infallible except for data-related errors:
NeoDateTimeFormatter::create_ymdt_with_options_v1(
*locale.get(),
options
)
// Alternative C:
DateTimeFieldSetBuilder builder;
builder.length = std::optional<NeoDateTimeLength>(NeoDateTimeLength::Medium);
builder.time_precision = std::optional<TimePrecision>(TimePrecision::Minute);
builder.alignment = std::optional<DateTimeAlignment>(DateTimeAlignment::Auto);
builder.year_style = std::optional<YearStyle>(YearStyle::Auto);
// Fails if the builder contains options incompatible with the field set, as well as data-related errors:
NeoDateTimeFormatter::create_ymdt(*locale.get(), options) |
I will also take feedback on the format functions. I intend to add the following format functions:
We don't have Also, please notice that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like the right approach!
#5940
This is the DateTime version. I plan to do basically the exact same design for:
See datetime.cpp for usage examples.
This supports both a builder API and a comprehensive static field set API by using the
with_fset
function I added.It is all in the FFI crate, which means this is less likely to conflict with some of the other open PRs in the datetime crate (CC @robertbastian).