Fix locale formatting for %c and %r

[scarf005]

Summary

• fixes Invalid datetime format for korean(ko_KR) #985

Breaking changes

%c uses locale's date and time (%x, %X, %r)

assert_eq!(
	dt.format_localized("%c", Locale::ja_JP).to_string(),
	"2001年07月08日 00時34分60秒"
);
assert_eq!(
    dt.format_localized("%c", Locale::ko_KR).to_string(),
    "2001년 07월 08일 (일) 오전 12시 34분 60초"
);

%r uses locale's 12 hour clock time

assert_eq!(dt.format_localized("%r", Locale::ko_KR).to_string(), "오전 12시 34분 60초");
assert_eq!(dt.format_localized("%r", Locale::ja_JP).to_string(), "午前12時34分60秒");

Library changes

new macro: locale_match_ampm!

uses hardcoded list of locales to find T_FMT_AMPM due to chronotope/pure-rust-locales#4

new tests: test_strftime_localized_korean and test_strftime_localized_japanese

will test some common locale related strftime format specifiers.

[djc]

Thanks for fixing the clippy issue!

[djc]

I think this should get a clear comment to explain in high-level terms what it's trying to achieve, as that is pretty unclear to me. I'm not sure I agree that the stacked macro approach is the best option here...

[scarf005]

alright, i think description for it would be similar to locale_match_ampm! macro, but will try splitting it up for high and low level terms.

as with the stacked macro, do you mean using both locale_match_ampm and expand_ampm macro? i too am not sure about whether it's best approach. however locale_match wouldn't work due to chronotope/pure-rust-locales#4 so i guess we're probably stuck with this solution for now.

[scarf005]

added more docstring at 3f0aafd. could you take a look?

[esheppa]

Thanks for this @scarf005

[esheppa]

Do we ever want to have a possible non-localized %r equivalent? Or do we think that it is probably not as useful as the localized version and hence it makes most sense to have the localized version only?

[djc]

Reading the referenced issue, notice the comment in chronotope/pure-rust-locales#4 (comment):

There is something utterly important that you must know about the current implementation though: I wanted the compiler to optimize away unused code. Every constant or function that is not used in the final binary (the user's binary) should (and must) be stripped by the compiler. This is because we don't want to store all the information for all the languages on every program.

It looks to me like the current setup here ends up referencing a long list of locales in our code, meaning that all of those will essentially be compiled into the chrono library if the localization feature is enabled. So I don't think this is the right approach.

[scarf005]

iirc locale_match_ampm will expand roughly as same as how match_locale macro would have, original locale_match is:

#[macro_export]
macro_rules! locale_match {
    ($locale:expr => $($item:ident)::+) => {{
        match $locale {
            $crate::Locale::POSIX => $crate::POSIX::$($item)::+,
            $crate::Locale::aa_DJ => $crate::aa_DJ::$($item)::+,
            $crate::Locale::aa_ER => $crate::aa_ER::$($item)::+,
			(...around 290 items more)

the difference between match_locale and match_locale_ampm is that ampm one

• removes 3 locales that misses LC_LANG::T_FMT_AMPM
• has default fallback value

[scarf005]

looks like rust msrv is too low to support 2021, should i raise it?

[jtmoon79]

looks like rust msrv is too low to support 2021, should i raise it?

Relates to #1003 and #995

[scarf005]

thanks. i'll rebase the PR once msrv issue is fixed.

[pitdicker]

thanks. i'll rebase the PR once msrv issue is fixed.

The time has come 😄

[djc]

Actually, this PR targets main, which doesn't have the MSRV bump yet.

[djc]

So is there as reason this cannot target 0.4.x?

[scarf005]

oh, i see, should've made a PR to 0.4.x. closing in favor of a new one targeting 0.4.x.