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

Render dates in locale-aware fashion #190

Merged
merged 1 commit into from Feb 1, 2019
Merged

Render dates in locale-aware fashion #190

merged 1 commit into from Feb 1, 2019

Conversation

camdez
Copy link
Contributor

@camdez camdez commented Feb 1, 2019

Recent changes (bef524b) rendered the date filter less friendly to locales by enforcing format strings rather than using built-in locale-aware formats for dates / times. Using the named, built-in formats allows for proper expression of localized dates / times, which may not naturally follow the same component order as (e.g.) a format string that is natural for English.

A salient example would be rendering 2014-03-01 into Chinese ("{{d|date:longDate:zh}}"); prior to this code change, the result is 三月 01, 2014 (which is super odd if, arguably, comprehensible) rather than the much more natural 2014年3月1日 yielded after the patch.

In the interests of not assuming the reader is familiar with Chinese, it's likely enough to explain that Chinese dates are always written from the largest unit (in this case the year) to the smallest (the day), which is not possible when the format is hardcoded to be "MMMM dd, yyyy".

@yogthos yogthos merged commit 44a7475 into yogthos:master Feb 1, 2019
@yogthos
Copy link
Owner

yogthos commented Feb 1, 2019

Hi, that makes sense to me. Originally, I wanted to keep formatting in the same style as Joda for backwards compatibility, but I didn't consider this case. It's worth noting that the date formats change between JDK 8 to and JDK 10:

FAIL in (filter-date) (core_test.clj:693)
expected: (= "12:00 AM" (render "{{d|date:shortTime:en_US}}" {:d firstofmarch}))
  actual: (not (= "12:00 AM" "00:00"))

lein test :only selmer.core-test/filter-date

FAIL in (filter-date) (core_test.clj:695)
expected: (= "3/1/14" (render "{{d|date:shortDate:en_US}}" {:d firstofmarch}))
  actual: (not (= "3/1/14" "2014-03-01"))

lein test :only selmer.core-test/filter-date

FAIL in (filter-date) (core_test.clj:696)
expected: (= "14-3-1" (render "{{d|date:shortDate:zh}}" {:d firstofmarch}))
  actual: (not (= "14-3-1" "2014/3/1"))

lein test :only selmer.core-test/filter-date

FAIL in (filter-date) (core_test.clj:697)
expected: (= "3/1/14 12:00 AM" (render "{{d|date:shortDateTime:en_US}}" {:d firstofmarch}))
  actual: (not (= "3/1/14 12:00 AM" "2014-03-01 00:00"))

lein test :only selmer.core-test/filter-date

FAIL in (filter-date) (core_test.clj:698)
expected: (= "14-3-1 上午12:00" (render "{{d|date:shortDateTime:zh}}" {:d firstofmarch}))
  actual: (not (= "14-3-1 上午12:00" "2014/3/1 上午12:00"))

lein test :only selmer.core-test/filter-date

FAIL in (filter-date) (core_test.clj:700)
expected: (= "March 1, 2014" (render "{{d|date:longDate:en_US}}" {:d firstofmarch}))
  actual: (not (= "March 1, 2014" "2014 Mar 1"))

@camdez
Copy link
Contributor Author

camdez commented Feb 1, 2019

@yogthos Thanks for the merge! I had also noticed that they changed but I figured it would be ok as they had changed in bef524b as well... I actually discovered this change when one of my users reported a change in date format from mm/dd/yy to dd/mm/yy (which can be a rather sneaky change...especially if it's in a data export or something along those lines).

Reflecting on it a bit, it seemed reasonable to me that a consumer of this code couldn't necessarily assume that the format would be perfectly fixed when using the named formats (especially if the machine's locale is allowed to come into play), but rather would want to use a format string if a precise, fixed format was expected.

I don't know if that resonates with others, but that was my basic thinking.

@yogthos
Copy link
Owner

yogthos commented Feb 1, 2019

I've updated the tests for JDK 10, and I also think it's best to rely on the default JDK formatting behavior here. Since it's possible to specify an explicit format, I think that addresses the case where default formats aren't sufficient. I just pushed out 1.12.6 with the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants