-
Notifications
You must be signed in to change notification settings - Fork 58
Intl4x API refactorings. #1023
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
base: main
Are you sure you want to change the base?
Intl4x API refactorings. #1023
Conversation
Summary of ChangesHello @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 significantly refactors 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 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 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request is a great simplification of the intl4x API. Removing the central Intl class and promoting the individual formatting classes to be top-level makes the library more modular and easier to understand and use. The renames of Type to ListType and PluralType are also excellent changes that prevent name collisions.
While the refactoring is a positive change, it introduces several breaking changes that should be clearly documented for users of the library, for example in the PR description and the package's changelog. These include:
- The removal of the
Intlclass. - The removal of
ofDateTime,ofScript,ofCurrency, andofCalendarmethods fromDisplayNames. - Renaming and removal of some
Calendarenum values.
I've added a few comments with suggestions for improvements, mostly related to code style, consistency, and documentation, following the repository's contribution guidelines.
| final CaseMappingImpl _caseMappingImpl; | ||
|
|
||
| const CaseMapping._(this._caseMappingImpl); | ||
| CaseMapping({Locale? locale}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The repository's style guide requires documentation for all public members.1 Please add a doc comment for the CaseMapping constructor explaining what it does. If locale is not provided, it defaults to the system locale.
For example:
/// Constructs a [CaseMapping] instance for the given [locale].
///
/// If [locale] is not provided, the system's locale is used.This also applies to the new public constructors in Collation, DateTimeFormat, DisplayNames, ListFormat, NumberFormat, and PluralRules.
Style Guide References
Footnotes
-
The style guide mandates that all public members should have documentation. ↩
PR HealthLicense Headers ✔️
All source files should start with a license header. This check can be disabled by tagging the PR with API leaks ✔️The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
This check can be disabled by tagging the PR with Breaking changes ✔️
This check can be disabled by tagging the PR with
Coverage
|
| File | Coverage |
|---|---|
| pkgs/intl4x/example/bin/example.dart | 💔 Not covered |
| pkgs/intl4x/lib/case_mapping.dart | 💚 100 % |
| pkgs/intl4x/lib/collation.dart | 💔 0 % ⬇️ NaN % |
| pkgs/intl4x/lib/datetime_format.dart | 💔 Not covered |
| pkgs/intl4x/lib/display_names.dart | 💔 Not covered |
| pkgs/intl4x/lib/list_format.dart | 💚 100 % |
| pkgs/intl4x/lib/number_format.dart | 💔 Not covered |
| pkgs/intl4x/lib/plural_rules.dart | 💔 Not covered |
| pkgs/intl4x/lib/src/case_mapping/case_mapping.dart | 💚 100 % |
| pkgs/intl4x/lib/src/collation/collation.dart | 💔 88 % ⬇️ 13 % |
| pkgs/intl4x/lib/src/collation/collation_4x.dart | 💚 85 % ⬆️ 9 % |
| pkgs/intl4x/lib/src/collation/collation_ecma.dart | 💔 Not covered |
| pkgs/intl4x/lib/src/collation/collation_options.dart | 💚 9 % |
| pkgs/intl4x/lib/src/collation/collation_stub.dart | 💔 0 % ⬇️ NaN % |
| pkgs/intl4x/lib/src/collation/collation_stub_4x.dart | 💔 Not covered |
| pkgs/intl4x/lib/src/datetime_format/datetime_format.dart | 💔 78 % ⬇️ 5 % |
| pkgs/intl4x/lib/src/datetime_format/datetime_format_ecma.dart | 💔 Not covered |
| pkgs/intl4x/lib/src/datetime_format/datetime_format_impl.dart | 💔 83 % ⬇️ 2 % |
| pkgs/intl4x/lib/src/datetime_format/datetime_format_options.dart | 💚 40 % ⬆️ 28 % |
| pkgs/intl4x/lib/src/datetime_format/datetime_format_stub.dart | 💔 0 % ⬇️ NaN % |
| pkgs/intl4x/lib/src/datetime_format/datetime_format_stub_4x.dart | 💔 Not covered |
| pkgs/intl4x/lib/src/datetime_format/icu4x/date_formatter.dart | 💚 100 % ⬆️ 5 % |
| pkgs/intl4x/lib/src/datetime_format/icu4x/date_time_formatter.dart | 💚 100 % ⬆️ 5 % |
| pkgs/intl4x/lib/src/datetime_format/icu4x/datetime_format_4x.dart | 💚 98 % ⬆️ 7 % |
| pkgs/intl4x/lib/src/datetime_format/icu4x/time_formatter.dart | 💚 9 % |
| pkgs/intl4x/lib/src/display_names/display_names.dart | 💚 80 % ⬆️ 37 % |
| pkgs/intl4x/lib/src/display_names/display_names_4x.dart | 💚 95 % ⬆️ 38 % |
| pkgs/intl4x/lib/src/display_names/display_names_ecma.dart | 💔 Not covered |
| pkgs/intl4x/lib/src/display_names/display_names_impl.dart | 💚 100 % |
| pkgs/intl4x/lib/src/display_names/display_names_options.dart | 💚 17 % |
| pkgs/intl4x/lib/src/display_names/display_names_stub.dart | 💔 0 % ⬇️ NaN % |
| pkgs/intl4x/lib/src/display_names/display_names_stub_4x.dart | 💔 Not covered |
| pkgs/intl4x/lib/src/list_format/list_format.dart | 💔 75 % ⬇️ 10 % |
| pkgs/intl4x/lib/src/list_format/list_format_4x.dart | 💚 100 % |
| pkgs/intl4x/lib/src/list_format/list_format_ecma.dart | 💔 Not covered |
| pkgs/intl4x/lib/src/list_format/list_format_options.dart | 💚 50 % ⬆️ 200 % |
| pkgs/intl4x/lib/src/list_format/list_format_stub.dart | 💔 0 % ⬇️ NaN % |
| pkgs/intl4x/lib/src/list_format/list_format_stub_4x.dart | 💔 Not covered |
| pkgs/intl4x/lib/src/locale/locale.dart | 💔 25 % ⬇️ 25 % |
| pkgs/intl4x/lib/src/locale/locale_4x.dart | 💚 73 % ⬆️ 71 % |
| pkgs/intl4x/lib/src/locale/locale_ecma.dart | 💔 Not covered |
| pkgs/intl4x/lib/src/number_format/number_format.dart | 💔 30 % ⬇️ 64 % |
| pkgs/intl4x/lib/src/number_format/number_format_ecma.dart | 💔 Not covered |
| pkgs/intl4x/lib/src/number_format/number_format_options.dart | 💚 31 % |
| pkgs/intl4x/lib/src/options.dart | 💚 100 % |
| pkgs/intl4x/lib/src/plural_rules/plural_rules.dart | 💔 86 % ⬇️ 14 % |
| pkgs/intl4x/lib/src/plural_rules/plural_rules_4x.dart | 💚 100 % |
| pkgs/intl4x/lib/src/plural_rules/plural_rules_ecma.dart | 💔 Not covered |
| pkgs/intl4x/lib/src/plural_rules/plural_rules_options.dart | 💚 22 % |
| pkgs/intl4x/lib/src/plural_rules/plural_rules_stub.dart | 💔 0 % ⬇️ NaN % |
| pkgs/intl4x/lib/src/plural_rules/plural_rules_stub_4x.dart | 💔 Not covered |
| pkgs/intl4x/lib/src/utils.dart | 💚 100 % ⬆️ 50 % |
This check for test coverage is informational (issues shown here will not fail the PR).
This check can be disabled by tagging the PR with skip-coverage-check.
Changelog Entry ✔️
| Package | Changed Files |
|---|
Changes to files need to be accounted for in their respective changelogs.
This check can be disabled by tagging the PR with skip-changelog-check.
robertbastian
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
most of the gemini comments are actionable as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also test the string extension method
|
/gemini review |
Package publishing
Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a significant and welcome refactoring of the intl4x API. The removal of the Intl object and the direct integration of options into the formatters' constructors greatly simplifies the API and improves usability. The added documentation, examples, and conformance tests are valuable additions that enhance the package's quality and maintainability. The changes align well with modern Dart practices, such as using expression bodies (=>) and extension types. I've found a few minor issues, mostly related to documentation typos and a small error in an example, which are detailed in the comments.
| /// DisplayNames(locale: Locale.parse('es-419')) | ||
| /// .ofLanguage(Locale.parse('de')); // Alemania |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example seems to have a copy-paste error. To get the name of the region 'Germany' in Spanish, you should use ofRegion('DE'). The current code ofLanguage(Locale.parse('de')) would return the name of the German language ('alemán'), not the region name ('Alemania').
/// DisplayNames(locale: Locale.parse('es-419'))
/// .ofRegion('DE'); // Alemania| /// | ||
| library; | ||
|
|
||
| import 'display_names.dart'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// The comparison rules are configured using the provided parameters: | ||
| /// | ||
| /// * [locale]: The specific locale to use for comparison. If `null`, the | ||
| /// system's current locale is determined used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// for a string, or sorting a list of strings. The | ||
| /// [CollationOptions.sensitivity] regulates how exact the comparison should | ||
| /// be. Setting [CollationOptions.numeric] means that numbers are not sorted | ||
| /// alphbetically, but by their value. The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General API refactorings.
Intlobject, as it only served as a wrapper ofLocale. So the...Formatobjects are now the top-level things....Optionsobjects from the public API, as they make it more verbose. Users now directly input the options to the top-level...Formatconstructor.=>instead of block bodies for methods where possible.DateTimeformatting. This shows many discrepancies for now, which need to be resolved before this PR can be merged - or we need to decide they are fine and remove the conformance testing.Fixes #1027
Contribution guidelines:
dart format.Many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.
Note: The Dart team is trialing Gemini Code Assist. Don't take its comments as final Dart team feedback. Use the suggestions if they're helpful; otherwise, wait for a human reviewer.