Add validation/fixes for time formatting strings #5
Conversation
|
If you don't mind, can you get someone from chrono who review the PR there to review also this one here? It's just to be sure I'm not missing some functional aspect |
|
Good point. To be honest a corresponding PR doesn't exist yet... I'll see what I can do. |
generate-api/src/main.rs
Outdated
| @@ -66,3 +68,32 @@ fn main() -> Result<()> { | |||
|
|
|||
| Ok(()) | |||
| } | |||
|
|
|||
| fn validate(objects: &mut Vec<Object>) { | |||
There was a problem hiding this comment.
I feel like validate() is not a great name for something that takes a mutable reference?
generate-api/src/main.rs
Outdated
|
|
||
| fn validate_t_fmt_ampm(objects: &mut Vec<Object>) { | ||
| for object in objects.iter_mut() { | ||
| if object.name == "LC_TIME" { |
There was a problem hiding this comment.
Can we do an early return for != "LC_TIME" to save on rightward drift?
generate-api/src/main.rs
Outdated
| validate_t_fmt_ampm(objects); | ||
| } | ||
|
|
||
| fn validate_t_fmt_ampm(objects: &mut Vec<Object>) { |
There was a problem hiding this comment.
Maybe add a docstring here that explains what this is trying to do and why?
generate-api/src/main.rs
Outdated
| let value = match am_pm_empty { | ||
| true => vec![Value::String(String::new())], |
There was a problem hiding this comment.
Should we still be pushing a value if !am_pm_empty? The logic here doesn't really make sense to me.
There was a problem hiding this comment.
This duplicates what other locales do which don't have a 12-hour format for the time.
In those locales AM_PM has two empty strings for AM and PM, and T_FMT_AMPM has an empty string.
generate-api/src/main.rs
Outdated
| } | ||
| } | ||
| for (key, ref mut value) in object.values.iter_mut() { | ||
| match (key.as_str(), value) { |
There was a problem hiding this comment.
Could also do with some comments, I guess?
|
@djc Thank you for reviewing this quickly! |
|
Released in 0.6.0 |
|
Thank you for all the work and picking it up this quickly. |
|
Thanks to you for contributing to the internalization of chrono ❤️ It was a huge hurdle for me back in the days |
In chronotope/chrono#1058 in chrono we run into two issues.
Three locales are missing
T_FMT_AMPM(ff_SN,km_KH,ug_CN). This makes us unable to uselocale_match!forT_FMT_AMPM. We could hack around it, but it seems better to me to fix the locales.If I look at the three cases, the ones that define
AMPMshould have"%l:%M:%S %p"and otherwise use"".The other issue is more of a detail, but something I would appreciate.
Since chronotope/chrono#1152 we try to parse a format string without allocating. For this we switch from parsing the user-supplied format string to a localized format string when it encounters
%x,%Xor%c(and hopefully soon on%r).The iterator is able to hold a reference to the original string, and to the localized string that is being parsed.
Now if the localized string references another localized string, such as
%rinside%c, we would have to add the ability to switch to parsing a third format string.It would be great
pure-rust-localescould inline the format string of%x,%Xand%rintoD_T_FMT.But if this is not an option, we can work around it.