daypack-dev / timere

OCaml date time handling and reasoning suite
MIT License
68 stars 7 forks source link

feature used: conversion from Timere to Ptime value #29

Closed gasche closed 3 years ago

gasche commented 3 years ago

I'm interfacing with a library (icalendar) that expects Ptime.t and Ptime.Span.t values. I wrote the following conversion functions:

let ptime_of_timere (v : Timere.Date_time.t) : Ptime.t =
  Timere.Date_time.to_timestamp_float_single v
  |> Ptime.of_float_s
  |> Option.get

let ptime_span_of_timere_duration (v : Timere.Duration.t) : Ptime.Span.t =
  Timere.Duration.to_span v
  |> Timere.Span.to_float
  |> Ptime.Span.of_float_s
  |> Option.get

I found it non-obvious that I would have to use float as the intermediary data structure. One thing that is tricky I think is that Timere offers to export timestamp into a int64 * int basically, and Ptime has a version that expects a int * int64, but the types are used to mean very different things (Timere uses seconds and nanoseconds, Ptime uses days and picoseconds).

In order to not add a dependency on Ptime in Timere (if you don't want to), those could come in a separate timere-ptime package (the "modern" equivalent of timere.ptime subpackages of ocamlfind times) that would be alone in depending on Ptime.

(Note: If you were interested in integrating this, I suppose the Option.get would go away to be replaced by returning an option. This also depends on whether there are values representable by Timere that cannot be safely converted to Ptime.)

darrenldl commented 3 years ago

Timere already depends on Ptime, so that is not a concern (didn't want to/why reimplement Gregorian calendar when a perfectly nice implementation is around : D).

And yep, Timere already has internal lossless conversion from int64 to Ptime.t (where most of the somewhat messy maths happens for translating between the two representations).

I'll add ns support to that and export translation of both directions in the main interface, say in Timere.Utils?

(Still trying to work out ISO ordinal date and ISO week date atm, so will get to this shortly)

gasche commented 3 years ago

One related remark: if I remember correctly, the Date_time.{of,to}_timestamp_float{,_single} functions are not documented, so I was not sure (until I tested) that they actually used the same representation as Ptime, which is very clear about the fact that its (double) floating-point numbers represent a number of seconds. This could be clarified.

darrenldl commented 3 years ago

One related remark: if I remember correctly, the Date_time.{of,to}_timestamp_float{,_single} functions are not documented, so I was not sure (until I tested) that they actually used the same representation as Ptime, which is very clear about the fact that its (double) floating-point numbers represent a number of seconds. This could be clarified.

Yep, added - should I also rename them to *_float_s*?

And I've added the ptime utils to Timere.Utils, I'll figure out a test suite later.

gasche commented 3 years ago

Yep, added - should I also rename them to *_float_s*?

Why not? When we are not sure, taking inspiration from Daniel Bünzli's API choices sounds very reasonable.