Sitelet https://web.archive.org/web/20210812214405/https://github.com/flutter/flutter/pull/86844
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

[gen_l10n] to handle arbitrary DateFormat patterns #86844

Merged
merged 2 commits into from Aug 12, 2021

Conversation

@asashour
Copy link
Contributor

@asashour asashour commented Jul 22, 2021 •

Fixes #77646

Use DateFormat constructor with any arbitrary pattern. However, I am not sure if it should throw an exception in some particular case.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@google-cla google-cla bot added the cla: yes label Jul 22, 2021
@jonahwilliams jonahwilliams requested a review from HansMuller Jul 22, 2021
Copy link
Contributor

@HansMuller HansMuller left a comment •

The reason we used the DateFormat factory constructors, instead of just allowing the developer to specifying an arbitrary format string, was because it eliminated the possibility of a runtime error due to a bad format string. I agree that it's reasonable to allow developers to specify an arbitrary format string - as an alternative - however I don't think we should stop recognizing the ones that correspond to DateFormat factory constructors. I assume that most apps, most of the time, will be able to use the formats for which there is a factory constructor.

We want to avoid breaking anything and to encourage developers to continue to use the standard formats when possible. For example you could introduce a placeholder attribute like isCustomFormat, ("isCustomDateFormat": true) which would have to be present if format's value was an arbitrary DateFormat string. There are certainly more convenient ways to express this, but we'd prefer that developers only use custom formats when that's necessary. And when they do, they should do so explicitly.

Sorry about all of the direction; hope this makes sense.

@asashour
Copy link
Contributor Author

@asashour asashour commented Jul 24, 2021

Thanks a lot for your feedback and detailed guidance.

This would be better, to try help developers to use standard format whenever possible.

@asashour asashour force-pushed the asashour:77646_DateFormat branch from 2fefcef to b7709dc Jul 24, 2021
@christopherfujino
Copy link
Contributor

@christopherfujino christopherfujino commented Aug 12, 2021

@asashour what is the current state of this PR?

Copy link
Contributor

@HansMuller HansMuller left a comment

LGTM

@HansMuller HansMuller merged commit 5848a16 into flutter:master Aug 12, 2021
81 checks passed
81 checks passed
@flutter-dashboard
Linux analyze
Details
@flutter-dashboard
Linux build_aar_module_test
Details
@flutter-dashboard
Linux build_tests_1_2
Details
@flutter-dashboard
Linux build_tests_2_2
Details
@flutter-dashboard
Linux customer_testing
Details
@flutter-dashboard
Linux firebase_abstract_method_smoke_test
Details
@flutter-dashboard
Linux firebase_android_embedding_v2_smoke_test
Details
@flutter-dashboard
Linux firebase_release_smoke_test
Details
@flutter-dashboard
Linux flutter_plugins
Details
@flutter-dashboard
Linux fuchsia_precache
Details
@flutter-dashboard
Linux module_custom_host_app_name_test
Details
@flutter-dashboard
Linux module_host_with_custom_build_test
Details
@flutter-dashboard
Linux module_test
Details
@flutter-dashboard
Linux plugin_test
Details
@flutter-dashboard
Linux skp_generator
Details
@flutter-dashboard
Linux tool_integration_tests_1_4
Details
@flutter-dashboard
Linux tool_integration_tests_2_4
Details
@flutter-dashboard
Linux tool_integration_tests_3_4
Details
@flutter-dashboard
Linux tool_integration_tests_4_4
Details
@flutter-dashboard
Linux tool_tests_commands
Details
@flutter-dashboard
Linux tool_tests_general
Details
@flutter-dashboard
Linux web_long_running_tests_1_5
Details
@flutter-dashboard
Linux web_long_running_tests_2_5
Details
@flutter-dashboard
Linux web_long_running_tests_3_5
Details
@flutter-dashboard
Linux web_long_running_tests_4_5
Details
@flutter-dashboard
Linux web_long_running_tests_5_5
Details
@flutter-dashboard
Linux web_tests_0
Details
@flutter-dashboard
Linux web_tests_1
Details
@flutter-dashboard
Linux web_tests_2
Details
@flutter-dashboard
Linux web_tests_3
Details
@flutter-dashboard
Linux web_tests_4
Details
@flutter-dashboard
Linux web_tests_5
Details
@flutter-dashboard
Linux web_tests_6
Details
@flutter-dashboard
Linux web_tests_7_last
Details
@flutter-dashboard
Linux web_tool_tests
Details
@flutter-dashboard
Mac build_aar_module_test
Details
@flutter-dashboard
Mac build_ios_framework_module_test
Details
@flutter-dashboard
Mac build_tests_1_4
Details
@flutter-dashboard
Mac build_tests_2_4
Details
@flutter-dashboard
Mac build_tests_3_4
Details
@flutter-dashboard
Mac build_tests_4_4
Details
@flutter-dashboard
Mac customer_testing
Details
@flutter-dashboard
Mac dart_plugin_registry_test
Details
@flutter-dashboard
Mac module_custom_host_app_name_test
Details
@flutter-dashboard
Mac module_host_with_custom_build_test
Details
@flutter-dashboard
Mac module_test
Details
@flutter-dashboard
Mac module_test_ios
Details
@flutter-dashboard
Mac plugin_test
Details
@flutter-dashboard
Mac tool_integration_tests_1_4
Details
@flutter-dashboard
Mac tool_integration_tests_2_4
Details
@flutter-dashboard
Mac tool_integration_tests_3_4
Details
@flutter-dashboard
Mac tool_integration_tests_4_4
Details
@flutter-dashboard
Mac tool_tests_general
Details
@flutter-dashboard
Mac web_tool_tests
Details
@wip
WIP Ready for review
Details
@flutter-dashboard
Windows build_aar_module_test
Details
@flutter-dashboard
Windows build_tests_1_3
Details
@flutter-dashboard
Windows build_tests_2_3
Details
@flutter-dashboard
Windows build_tests_3_3
Details
@flutter-dashboard
Windows customer_testing
Details
@flutter-dashboard
Windows module_custom_host_app_name_test
Details
@flutter-dashboard
Windows module_host_with_custom_build_test
Details
@flutter-dashboard
Windows module_test
Details
@flutter-dashboard
Windows plugin_test
Details
@flutter-dashboard
Windows tool_integration_tests_1_5
Details
@flutter-dashboard
Windows tool_integration_tests_2_5
Details
@flutter-dashboard
Windows tool_integration_tests_3_5
Details
@flutter-dashboard
Windows tool_integration_tests_4_5
Details
@flutter-dashboard
Windows tool_integration_tests_5_5
Details
@flutter-dashboard
Windows tool_tests_commands
Details
@flutter-dashboard
Windows tool_tests_general
Details
@flutter-dashboard
Windows web_tool_tests
Details
@cirrus-ci
analyze-linux Task Summary
Details
@flutter-dashboard
ci.yaml validation .ci.yaml validation
Details
@google-cla
cla/google All necessary CLAs are signed
@cirrus-ci
customer_testing-linux Task Summary
Details
@cirrus-ci
docs-linux Task Summary
Details
@flutter-dashboard
flutter-gold All golden file tests have passed.
Details
@flutter-dashboard
luci-flutter
Details
@cirrus-ci
tool_tests-commands-linux Task Summary
Details
@cirrus-ci
tool_tests-general-linux Task Summary
Details
@asashour asashour deleted the asashour:77646_DateFormat branch Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants