Infer the zone datetime in FFI when possible #6517
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @sffc, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request addresses issue #6378 by inferring the zone datetime in FFI when possible. The changes primarily involve modifying the DateTimeInputUnchecked struct and updating the FFI bindings for ZonedDateTimeFormatter to automatically set the time zone name timestamp when it's not explicitly provided. This enhancement improves FFI support, particularly for ZonedDateTimeFormatter, although it doesn't yet extend to ZonedDateFormatter and ZonedTimeFormatter.
Highlights
- FFI Enhancement: The pull request enhances FFI support by enabling the inference of zone datetime when possible, specifically targeting
ZonedDateTimeFormatter. - Automatic Time Zone Timestamp: The changes introduce automatic setting of the time zone name timestamp in
ZonedDateTimeFormatterwhen it's not explicitly provided, usingZoneNameTimestamp::from_date_time_iso. - Code Modification: The code has been modified to use
date_in_calendarinstead ofdateinset_date_fields_uncheckedfor clarity and consistency.
Changelog
Click here to see the changelog
- components/datetime/src/format/input.rs
- Renamed
inputtodate_in_calendarinset_date_fields_uncheckedfor better readability.
- Renamed
- ffi/capi/src/zoned_date_formatter.rs
- Renamed
datetodate_in_calendarinwrite_to_diplomatfor better readability.
- Renamed
- ffi/capi/src/zoned_date_time_formatter.rs
- Renamed
datetodate_in_calendarinwrite_to_diplomatfor better readability. - Added logic to automatically set
time_zone_name_timestampusingZoneNameTimestamp::from_date_time_isowhen not explicitly provided.
- Renamed
- ffi/dart/test/icu_test.dart
- Added test case with
customZDTandcustomZoneto test the new functionality. - Changed
ZonedDateTimeFormatter.genericLongtoZonedDateTimeFormatter.specificLongin one of the tests. - Added a new expectation to test
ZonedDateTimeFormatter.genericLongwithcustomZDTandcustomZone.
- Added test case with
- tools/make/codegen/templates/zoned_formatter.rs.jinja
- Renamed
datetodate_in_calendarin the generated code for better readability. - Added logic to automatically set
time_zone_name_timestampusingZoneNameTimestamp::from_date_time_isowhen not explicitly provided in the generated code.
- Renamed
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
In zones of time, a dance,
FFI's grace, a hopeful chance.
DateTime's form,
Weathering the storm,
In code's embrace, we advance.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a change to infer the zone datetime in FFI when possible, which is a good step towards fixing #6378. The changes look reasonable, but there are a few areas that could be improved for clarity and correctness.
Summary of Findings
- Naming consistency: The naming of variables could be more consistent across the codebase. For example,
date_in_calendaris used in some places, whiledateis used in others. Standardizing this would improve readability. - Missing time zone name timestamp: The logic for setting the time zone name timestamp could be simplified and made more robust. Currently, it only sets the timestamp if it's explicitly provided, but it should also handle the case where it's not provided and can be inferred from the date and time.
Merge Readiness
The pull request makes progress towards resolving issue #6378, but it doesn't fully address the problem. There are also some areas for improvement in terms of naming consistency and logic simplification. I recommend addressing the high severity issue before merging. I am unable to approve this pull request, and users should have others review and approve this code before merging.
| else { | ||
| input.set_time_zone_name_timestamp(icu_time::zone::ZoneNameTimestamp::from_date_time_iso(&icu_time::DateTime { | ||
| date: date.0, | ||
| time: time.0 | ||
| })) | ||
| } |
There was a problem hiding this comment.
This else block is intended to infer the time zone name timestamp when it's not explicitly provided. However, it's unconditionally setting the timestamp based on the date and time. This might not always be the desired behavior. It would be better to check if the timestamp can be inferred from the zone ID before setting it here. Otherwise, the code might be overwriting a valid timestamp with an inferred one.
| else { | |
| input.set_time_zone_name_timestamp(icu_time::zone::ZoneNameTimestamp::from_date_time_iso(&icu_time::DateTime { | |
| date: date.0, | |
| time: time.0 | |
| })) | |
| } | |
| if let Some(zone_name_timestamp) = zone.zone_name_timestamp { | |
| input.set_time_zone_name_timestamp(zone_name_timestamp); | |
| } | |
| else if zone.id.is_some() { | |
| input.set_time_zone_name_timestamp(icu_time::zone::ZoneNameTimestamp::from_date_time_iso(&icu_time::DateTime { | |
| date: date.0, | |
| time: time.0 | |
| })) | |
| } | |
| else { | ||
| input.set_time_zone_name_timestamp(icu_time::zone::ZoneNameTimestamp::from_date_time_iso(&icu_time::DateTime { | ||
| date: date.0, | ||
| time: time.0 | ||
| })) | ||
| } |
There was a problem hiding this comment.
This else block is intended to infer the time zone name timestamp when it's not explicitly provided. However, it's unconditionally setting the timestamp based on the date and time. This might not always be the desired behavior. It would be better to check if the timestamp can be inferred from the zone ID before setting it here. Otherwise, the code might be overwriting a valid timestamp with an inferred one.
| else { | |
| input.set_time_zone_name_timestamp(icu_time::zone::ZoneNameTimestamp::from_date_time_iso(&icu_time::DateTime { | |
| date: date.0, | |
| time: time.0 | |
| })) | |
| } | |
| if let Some(zone_name_timestamp) = zone.zone_name_timestamp { | |
| input.set_time_zone_name_timestamp(zone_name_timestamp); | |
| } | |
| else if zone.id.is_some() { | |
| input.set_time_zone_name_timestamp(icu_time::zone::ZoneNameTimestamp::from_date_time_iso(&icu_time::DateTime { | |
| date: date.0, | |
| time: time.0 | |
| })) | |
| } | |
|
@gemini-code-assist What makes you think that the zone name timestamp can be inferred from the time zone ID |
|
Hi @sffc, Thanks for the question! My reasoning was based on the assumption that if the time zone ID is known, and the However, you're right to point out that this inference might not always be accurate or desired. There could be scenarios where the user wants to explicitly provide a I hope this clarifies my thought process. I'm open to discussing alternative approaches or refinements to this logic! |
|
But @gemini-code-assist, this PR is doing exactly what you asked for, I think. If the zone name timestamp was explicitly provided, we use it, and otherwise, we fall back to the formattable date and time. |
|
Hi @sffc, You're absolutely right! Thanks for pointing that out. I was a bit too quick in my previous assessment. I appreciate you implementing the logic as intended, prioritizing the explicitly provided timestamp when available and falling back to the formattable date and time when it's not. |
|
Ping @unicode-org/icu4x-owners |
|
(I did have this in my queue, I was just deep in V8 stuff) |
2 participants
Mostly fixes #6378
It doesn't make FFI work for ZonedDateFormatter and ZonedTimeFormatter since this trick only works for ZonedDateTimeFormatter, but at least this is progress.