Support %P (lowercase am/pm) glibc extension #3314
Conversation
There was a problem hiding this comment.
Thanks for the PR.
Looks good to me but please address the inline comments and update the docs (https://github.com/fmtlib/fmt/blob/master/doc/syntax.rst). Make sure to mention that %P is a non-standard extension.
include/fmt/chrono.h
Outdated
| case 'P': | ||
| handler.on_am_pm_lower(); | ||
| break; |
There was a problem hiding this comment.
nit: I'd suggest moving this after the 'p' case to keep the standard version first (and similarly for am_pm_lower functions below).
There was a problem hiding this comment.
The funny thing is that I wrote it in that order initially, then changed it to match what I observed at the start of the switch block, Y before y, without then checking to see if there was subsequent consistency in this way.
include/fmt/chrono.h
Outdated
| *out_++ = tm_hour() < 12 ? 'a' : 'p'; | ||
| *out_++ = 'm'; | ||
| } else { | ||
| format_localized('P'); |
There was a problem hiding this comment.
Please add a test case to ensure that it actually works with localized formatting.
There was a problem hiding this comment.
I don’t know how to do this. I wrote the code by blindly copying and modifying, not by deeply understanding what’s going on. 🙂
There was a problem hiding this comment.
Something along the lines of
Lines 756 to 757 in 0502936
but with
%P instead of %a and in a separate test case.
There was a problem hiding this comment.
Any updates? We'll need to have locale path tested before this can be merged.
Fun fact: in glibc you can also spell %P %#p, # meaning “swap case”. I haven’t implemented glibc’s ^ or # flags.
|
I’ve made the requested changes that I know how to, and request assistance on how to test the
I missed this because #3271 also missed this. |
|
Closing for now but feel free to resubmit with locale questions addressed. |
Fun fact: in glibc you can also spell %P %#p, # meaning “swap case”. I haven’t implemented glibc’s ^ or # flags (and don’t think there’s any particularly urgent need for it, so long as %P is around).
When #2544 broke this functionality for me and many other users of Waybar, I gathered from the response in #2811 that there was actively no interest in supporting glibc extensions. But now that #3271 has added the other important glibc extension (unpadded hours), I figured it might be worth making and trying submitting this patch after all.
The naming is arbitrary, and the code order for a quite possibly misguided form of consistency. I’m not fussed about it, feel free to amend it as you desire.