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

Add initial neo datetime FFI #6035

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Conversation

sffc
Copy link
Member

@sffc sffc commented Jan 25, 2025

#5940

This is the DateTime version. I plan to do basically the exact same design for:

  • Time
  • Date
  • GregorianDate
  • GregorianDateTime
  • ZonedDateTime
  • GregorianZonedDateTime

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).

@sffc sffc requested review from Manishearth and a team as code owners January 25, 2025 05:55
@sffc sffc requested a review from robertbastian January 25, 2025 05:56
Copy link
Member

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

@Manishearth
Copy link
Member

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.

@sffc
Copy link
Member Author

sffc commented Jan 26, 2025

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.

@sffc sffc requested a review from robertbastian January 26, 2025 05:46
@sffc sffc marked this pull request as draft January 26, 2025 05:47
@sffc
Copy link
Member Author

sffc commented Jan 26, 2025

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.

@sffc
Copy link
Member Author

sffc commented Jan 26, 2025

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)

@sffc
Copy link
Member Author

sffc commented Jan 26, 2025

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 _mv1 as an escape hatch, so I don't consider this a hard requirement.

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)

@sffc
Copy link
Member Author

sffc commented Jan 26, 2025

I will also take feedback on the format functions.

I intend to add the following format functions:

  1. format takes the same type as Rust format (matching calendar in GregorianDateTimeFormatter, any calendar in DateTimeFormatter)
  2. format_same_calendar has the same behavior as Rust format_same_calendar
  3. format_iso exists only in DateTimeFormatter

We don't have format_iso in Rust but I put it in C++ since we don't have overloading or traits.

Also, please notice that format_iso takes two positional arguments, the date and the time. I did not name the function format_iso_datetime. Instead, the required arguments come from the type. DateTimeFormatter's format functions take a date and a time as positional arguments.

Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants