[Dart] Fix some issues and upgrade to new DateTime API #509
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @mosuem, 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!
This pull request initiates a series of fixes and enhancements for the Dart executor, primarily focusing on updating its internationalization capabilities via the intl4x library. It includes updates to ICU versioning, improved handling of collation and date/time formatting options, and better error reporting for unsupported features. Dependency updates are also included to align with newer package versions and facilitate local development of the intl4x library.
Highlights
- ICU Version Update: The ICU version used in the Dart executor has been updated from 73 to 77, reflecting a newer version of the International Components for Unicode.
- Improved Collation Handling: The collation logic in
collator.dartnow includes robust error handling for unsupported "rules" and the "root" locale, and introduces a flexible "compare_type" parameter for different comparison behaviors. - Enhanced Date/Time Formatting: The date and time formatting in
datetime_format.darthas been significantly enhanced to support more granular options, including time zones and various date/time styles, and provides a more human-readable output for debugging. - Language Name Formatting Improvements: Specific error handling has been added to
lang_names.dartfor the unsupported "u-kr" extension in language label processing. - Number Formatting Adjustments: The number formatting in
numberformat.dartnow defaults tohalfEvenrounding mode and has removed thelocaleMatcherconfiguration, streamlining its options. - Dependency and Version Updates: The
intl4xlibrary version has been updated to0.13.0-wip, along with several other Dart package dependencies, ensuring compatibility and leveraging newer features. - Local Dependency Override: The
pubspec.yamlfile has been configured to use a local path override for theintl4xlibrary, facilitating local development and testing.
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 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
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.
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 number of fixes and feature enhancements for the Dart executor. Key changes include updating dependency versions, switching intl4x to a local path dependency, and expanding functionality in collation and date/time formatting. The collation logic now handles rules and different comparison types, while date/time formatting has been significantly reworked to support various styles and timezones. I've identified a couple of areas for improvement: some duplicated code in collator.dart that could be refactored, and a minor typo in an error message in datetime_format.dart. Overall, these are solid improvements.
| if (json.containsKey('rules')) { | ||
| outputLine['error_type'] = 'unsupported'; | ||
| outputLine['unsupported'] = 'unsupported_options'; | ||
| outputLine['error_message'] = 'Rules are not supported'; | ||
| return jsonEncode(outputLine); | ||
| } | ||
| final localeString = json['locale'] as String? ?? 'en'; | ||
| if (localeString == 'root') { | ||
| outputLine['error_type'] = 'unsupported'; | ||
| outputLine['unsupported'] = 'unsupported_options'; | ||
| outputLine['error_message'] = 'Locale `root` is unsupported'; | ||
| return jsonEncode(outputLine); | ||
| } |
There was a problem hiding this comment.
There's some code duplication in how unsupported options are handled for 'rules' and the 'root' locale. You could extract this logic into a local helper function within testCollation to improve code clarity and maintainability. For example:
String unsupported(String message) {
outputLine['error_type'] = 'unsupported';
outputLine['unsupported'] = 'unsupported_options';
outputLine['error_message'] = message;
return jsonEncode(outputLine);
}
if (json.containsKey('rules')) {
return unsupported('Rules are not supported');
}
// ...
if (localeString == 'root') {
return unsupported('Locale `root` is unsupported');
}| } catch (error) { | ||
| returnJson['unsupported'] = ': ${error.toString()}'; | ||
| returnJson['error'] = ': ${error.toString()}'; |
There was a problem hiding this comment.
The error message has a leading colon, which appears to be a typo. Replacing it with a descriptive word like 'Error' would make the error reporting more consistent with other parts of the codebase.
returnJson['error'] = 'Error: ${error.toString()}';
|
Hey Moritz, Thank you for working on this. There are some test failures here. Please try to resolve these so we can integrate. |
|
Yes, this is waiting for dart-lang/i18n#1003 to land. But even then, there will be still quite a lot of failures in the datetime formatting. Better than before though! |
mosuem
changed the title
|
@sven-oly this is ready for review - how does it look? |
|
General comments: I'm seeing errors /exceptions in the executors. |
|
Note that some test failures, e.g., Dart Web, ICU76.1, datetime_fmt have "UNKNOWN" as the label. This causes problems in the verifier. Here is content from TEMP_DATA/testOutput/dart_web/icu76/datetime_fmt_test.json. Even if there is a test failure or error, please set the label to be that of the original data. {"platform": {"platform": "Dart Web", "platformVersion": "v23.3.0", "icuVersion": "76.1", "intlVersion": "0.13.0"}, "test_environment": {"test_language": "dart_web", "executor": "node ../executors/dart/out/executor.js", "test_type": "datetime_fmt", "datetime": "09/05/2025, 22:39:27", "timestamp": 1757111967.5638766, "input_file": "../TEMP_DATA/testData/icu76/datetime_fmt_test.json", "icu_version": "76.1", "cldr_version": "CLDR version not specified", "test_count": 12936}, "test_type": "datetime_fmt", "tests": [**{"label": "UNKNOWN", "**input_data": "{"label": "00000", "locale": "en-US", "input_string": "2024-03-17T00:00:00Z", "options": {"timeZone": "America/Los_Angeles", "dateStyle": "short", "timeStyle": "short", "calendar": "gregory", "dateTimeFormatType": "standard"}, "tz_offset_secs": -25200.0, "original_input": "2024-03-16T17:00-07:00[America/Los_Angeles]", "hexhash": "4411425489f619054642a18e13b68490544728cb", "test_type": "datetime_fmt"}\n{... |
This builds on a newer intl4x DateTime API and adresses some of the points in dart-lang/i18n#1000 and dart-lang/i18n#998