`12hour` format is wrong for 12:00 hour

[dbp]

For the 12:00 - 13:00 hour (in 24hr time), if you use ~format:"{12hour:X}", it renders as 0, which is wrong: it should be 12. I'm assuming this is due to subtracting 12 from the hour, which is correct for 13 and up, but not for 12 (or, obviously, below).

[darrenldl]

Ah yeah, I never really used that part a lot and forgot to test the logic.

The code right now is:

         let hour = if hour = 0 then 12 else hour mod 12 in

which is obviously wrong now that I look at it again.

An am pm option is also missing from the pretty printing system - I'll add that too.

[dbp]

That would be great! Right now I have string concatenation with a manual check for the hour to add AM/PM :)

[darrenldl]

Does

         let hour = if hour = 0 || hour = 12 then 12 else hour mod 12 in

look more right?

I'll have to add test cases for this obviously, but I might as well ask while you're here : v

[dbp]

Yes that seems right? That is equivalent to what I think of as the “standard” conversation (mod being maybe neater, who knows!)

let hour = if hour = 0 then 12 else if hour > 12 then hour - 12 else hour

On Wed, Apr 10, 2024, at 7:17 PM, Darren Li wrote:

Does

let hour = if hour = 0 || hour = 12 then 12 else hour mod 12 in look more right?

I'll have to add test cases for this obviously, but I might as well ask while you're here : v

— Reply to this email directly, view it on GitHub https://github.com/daypack-dev/timere/issues/79#issuecomment-2048583386, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAELBJMLSU3TLOTKWG5PFXTY4XB6ZAVCNFSM6AAAAABGAZBU4SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBYGU4DGMZYGY. You are receiving this because you authored the thread.Message ID: @.***>

[darrenldl]

Added the 12hour fix (using the standard conversion you suggested), and also am/pm support:

utop # Timedesc.to_string ~format:"{12hour: X}" (Timedesc.now ());;
- : string = " 7"
─( 08:02:58 )─< command 3 >────────────────────────────────────────────────────────────────{ counter: 0 }─
utop # Timedesc.to_string ~format:"{12hour: X} {am/pm:XX}" (Timedesc.now ());;
- : string = " 7 AM"
─( 08:03:02 )─< command 4 >────────────────────────────────────────────────────────────────{ counter: 0 }─
utop # Timedesc.to_string ~format:"{12hour: X} {am/pm:xX}" (Timedesc.now ());;
- : string = " 7 aM"
─( 08:03:13 )─< command 5 >────────────────────────────────────────────────────────────────{ counter: 0 }─
utop # Timedesc.to_string ~format:"{12hour: X} {am/pm:xx}" (Timedesc.now ());;
- : string = " 7 am"
─( 08:03:16 )─< command 6 >────────────────────────────────────────────────────────────────{ counter: 0 }─
utop # Timedesc.to_string ~format:"{12hour: X} {am/pm:Xx}" (Timedesc.now ());;
- : string = " 7 Am"
─( 08:03:20 )─< command 7 >────────────────────────────────────────────────────────────────{ counter: 0 }─
utop # Timedesc.to_string ~format:"{12hour:0X} {am/pm:Xx}" (Timedesc.now ());;
- : string = "07 Am"
─( 08:03:23 )─< command 8 >────────────────────────────────────────────────────────────────{ counter: 0 }─
utop # Timedesc.to_string ~format:"{12hour:X} {am/pm:Xx}" (Timedesc.now ());;
- : string = "7 Am"
─( 08:03:28 )─< command 9 >────────────────────────────────────────────────────────────────{ counter: 0 }─
utop # Timedesc.to_iso8601 (Timedesc.now ());;
- : string = "2024-04-12T07:04:18.021737098Z"
─( 08:03:45 )─< command 10 >───────────────────────────────────────────────────────────────{ counter: 0 }─

Feel free to try pinning to main branch and see if it resolves your issues. Otherwise you can also wait till I've added the appropriate tests and published on opam.

[darrenldl]

Added an additional {am/pm:x.x.} format string, and added a bulk of the tests wanted: the test suite now covers {year} up to {12hour} and {am/pm} format strings currently). Will add the remaining test cases then a release can be made.

[darrenldl]

Submitted PR at opam repository: https://github.com/ocaml/opam-repository/pull/25754

Will close this issue when PR is merged.

[darrenldl]

PR merged, closing issue.