Repository navigation
Conversation
Summary of ChangesHello, 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 improves the Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
Customization To customize the 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 Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. 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. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements support for TimePrecision.minuteOptional in the ECMA datetime formatter, allowing minutes to be omitted when they are zero in 12-hour formats. It introduces a JS interop helper _assign to clone options and adds comprehensive test coverage for the new precision option. The review feedback highlights two key performance optimizations: caching the hour12 resolved option as a late final field to avoid expensive JS interop calls on every format invocation, and refactoring withTimeZone to accept an optional omitMinute parameter to prevent redundant object cloning.
| bool _shouldOmitMinute(DateTime datetime) => | ||
| _timePrecision == TimePrecision.minuteOptional && | ||
| datetime.minute == 0 && | ||
| (dateTimeFormat.resolvedOptions().hour12 ?? false); |
There was a problem hiding this comment.
Calling dateTimeFormat.resolvedOptions() on every format call is expensive because it is a JS interop call that queries the engine's internal formatter state and allocates a new JS object. Since the hour cycle configuration of the formatter does not change after construction, we should cache this value as a late final field in _FormatterECMA.
| bool _shouldOmitMinute(DateTime datetime) => | |
| _timePrecision == TimePrecision.minuteOptional && | |
| datetime.minute == 0 && | |
| (dateTimeFormat.resolvedOptions().hour12 ?? false); | |
| late final bool _is12Hour = dateTimeFormat.resolvedOptions().hour12 ?? false; | |
| bool _shouldOmitMinute(DateTime datetime) => | |
| _timePrecision == TimePrecision.minuteOptional && | |
| datetime.minute == 0 && | |
| _is12Hour; |
| final options = omitMinute | ||
| ? formatter._optionsJS.withoutMinute() | ||
| : formatter._optionsJS; | ||
| return _DateTimeFormat( | ||
| localeJS, | ||
| formatter._optionsJS.withTimeZone(timeZoneType, timeZone), | ||
| options.withTimeZone(timeZoneType, timeZone), | ||
| ); |
There was a problem hiding this comment.
Use the optimized withTimeZone method with the omitMinute parameter to avoid cloning the options object twice.
return _DateTimeFormat(
localeJS,
formatter._optionsJS.withTimeZone(
timeZoneType,
timeZone,
omitMinute: omitMinute,
),
);
Note
This PR was generated by an AI coding agent (Jetski) on behalf of @mosuem.
On ECMA (
datetime_format_ecma.dart),TimePrecision.minuteOptionalwas mapped tominute: 'numeric'via_style(timePrecision, TimePrecision.minute)becauseTimePrecision.minuteOptional.index >= TimePrecision.minute.index, so:00was always displayed even whendatetime.minute == 0in 12-hour locales (e.g.'4:00 AM'instead of'4 AM').In ICU4X (
TimePrecision::MinuteOptional), minutes are omitted whenminute == 0and the resolved hour cycle is 12-hour (h11/h12), and retained whenminute != 0or in 24-hour clock formats (14:00).Changes:
timePrecisioninto_FormatterECMA._fort,mdt,ymdt, andymdet.timePrecision == TimePrecision.minuteOptional,datetime.minute == 0, anddateTimeFormat.resolvedOptions().hour12 == true, format using options withminuteomitted (both for non-zoned and zoned formatters)._optionsviaObject.assignin_DateTimeJSOptions.withTimeZoneand_DateTimeJSOptions.withoutMinuteso the base options object is not mutated in place.TimePrecision.minuteOptionalindatetime_format_options.dart(which previously said "optional seconds" instead of "optional minutes").test/datetime_format_test.dartcoveringTimePrecision.minuteOptionalwith zero and non-zero minutes in 12-hour, 24-hour, and zoned formats.Verified locally:
dart analyze --fatal-infos,dart format --output=none --set-exit-if-changed .,dart test(VM), anddart test -p node -x ecmaUnsupportedpass.