search

haskell / time

A time library
http://hackage.haskell.org/package/time
Other
118 stars 78 forks source link

SecondOfDay and MinuteOfDay types #243

Closed NorfairKing closed 5 months ago

NorfairKing commented 1 year ago

I'm running into this bug: https://github.com/yesodweb/persistent/issues/1500 and I realise that actually I don't need sub-second or even sub-minute precision.

Perhaps time could have SecondOfDay and MinuteOfDay types that avoid this problem by construction.

AshleyYakeley commented 9 months ago

Hmm, could be useful.

Leaving aside the principle of not representing disallowed state, the more practical advantage of these types over TimeOfDay would be Read and Show instances, is that correct? Is this what caused the issue with persistent/sqlite?

NorfairKing commented 9 months ago

@AshleyYakeley I know we differ in opinion about this, so I won't argue about it.

What caused the issue with persistent/sqlite is that persistent/sqlite doesn't represent sub-second precision but TimeOfDay does.

AshleyYakeley commented 9 months ago

OK, but what actually happened that caused the problem?

NorfairKing commented 9 months ago

@AshleyYakeley I'm not sure. I'm guessing an upgrade somewhere but I can't tell of which package: time/sqlite/persistent...

AshleyYakeley commented 9 months ago

OK. Well let's leave this open, see if anyone has a real-world use-case for it.

NorfairKing commented 9 months ago

@AshleyYakeley I've already had a real-world use-case in https://social-dance.today. TimeOfDay doesn't roundtrip through sqlite so I needed to use SecondOfDay.

AshleyYakeley commented 9 months ago

Can you tell me why it's not round-tripping?

AshleyYakeley commented 9 months ago

Which way are you going, sqlite→TimeOfDay→sqlite, or TimeOfDay→sqlite→TimeOfDay? And if the latter, how are you constructing the initial TimeOfDay?

NorfairKing commented 9 months ago

@AshleyYakeley it's the latter. genValid :: Gen TimeOfDay -> (this part is "wrong" but it's baked into persistent) sqlite -> TimeOfDay

AshleyYakeley commented 5 months ago

I think this is better left for another package to do. It would introduce a lot of additional conversion functions, and generally I want to keep time focussed on time-related calculations rather than precise conceptual abstraction.