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

Zoned format ffi #6233

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

Zoned format ffi #6233

wants to merge 14 commits into from

Conversation

sffc
Copy link
Member

@sffc sffc commented Mar 6, 2025

Comment on lines +624 to +629
// TODO: What do we do if the zone variant is not set?
let zone = zone
.time_zone_id
.with_offset(zone.offset)
.at_time(at_time)
.with_zone_variant(zone.zone_variant.expect("TODO"));
Copy link
Member Author

@sffc sffc Mar 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed with @Manishearth and it seems the simplest solution is to return an error from this function if the option is not set.

Other solutions considered and eliminated:

  1. Have multiple time zone types: not great because it requires multiple formatter types
  2. Make zone variant non-optional: not great because you only need it for specific non-location formatting

@sffc sffc mentioned this pull request Mar 6, 2025
zone: &TimeZoneInfo,
write: &mut diplomat_runtime::DiplomatWrite,
) {
let at_time = zone.local_time.unwrap_or((date.0, time.0));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@robertbastian says this should be an error and not try to do anything clever

@sffc
Copy link
Member Author

sffc commented Mar 7, 2025

Finally, the NeoZonedDateTimeFormatter FFI is producing correct output. The amount of code in FFI instead of Rust is a little bit more than I'd like (a few lines of business logic that aren't just argument marshaling), but it's fine. We could move some of it into Rust in the future by adding a method to load "all data required for GenericLong formatting", for example, so that we don't need to figure out which load functions to use in each constructor.

I'm still a bit stuck on what to do in the format_iso function. Since we have only one NeoZonedDateTimeFormatter type, we cast all of the formatters into the most general one, which requires TimeZoneInfo<Full>, even if the zone style is more specific than that. This is incompatible with the notion that we return an error if the FFI TimeZoneInfo fields were not populated when we want them populated.

I'm trying to think of solutions. One is that I could make a custom input type specifically for FFI (one that implements all required icu_datetime::scaffold::GetField and, by necessity, icu_datetime::scaffold::UnstableSealed) which sets an error if a field is accessed but is in its None state. This is not pretty but I think it gets the job done? EDIT: Actually, no, it doesn't quite work because the fields are loaded eagerly.

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.

1 participant