search

arenadotio / pgx

A pure OCaml PostgreSQL client library
Other
122 stars 16 forks source link

Add Pgx_value_ptime with ptime converters #97

Closed brendanlong closed 3 years ago

brendanlong commented 4 years ago

Pgx_value_core has converters for Core.Date.t and Core.Time.t, but not everyone uses Core. We should add a new Pgx_value_ptime package with converters for Ptime.date and Ptime.time (and any other types that make sense to have converters).

brendanlong commented 4 years ago

This should be easy for someone else to do by looking at the code and tests in Pgx_value_core and basically just doing the same thing for Ptime. Let me know if you're interested and run into any issues.

jsthomas commented 3 years ago

I'm interested in working on this issue. I was hoping to get some feedback about whether I'm on the right track, since I'm a bit of a beginner with OCaml.

Currently my pgx_value_ptime.mli file looks like this:

type v = Pgx.Value.v [@@deriving compare, sexp_of]
type t = Pgx.Value.t [@@deriving compare, sexp_of]

include module type of Pgx.Value with type v := v and type t := t

val of_date : Ptime.date -> t
val to_date_exn : t -> Ptime.date
val to_date : t -> Ptime.date option

val of_time : ?tz_offset_s:Ptime.tz_offset_s -> Ptime.t -> t
val to_time_exn : t -> Ptime.t * Ptime.tz_offset_s
val to_time : t -> (Ptime.t * Ptime.tz_offset_s) option

val time_of_string : string -> Ptime.t * Ptime.tz_offset_s

Does the use of Ptime.t * Ptime.tz_offset_s tuples make sense? In the core version, it looks like Time contains a date, time of day, and timezone. However, Ptime.t describes a POSIX timestamp in UTC, so there needs to be some additional way of storing the timezone information.

I thought about using Ptime.date * Ptime.time instead; Ptime.time does carry timezone information. I ruled this out because Ptime.time has integer-valued seconds, which would mean a loss of precision.

brendanlong commented 3 years ago

@jsthomas Sorry, for some reason I didn't see the comment on here. One thing to consider is if it's necessary to pass the offset around at all. Another possible interface is to just always accept and return UTC timestamps. Although if Postgres stores offsets for some reason then having that be available in the interface is probably valuable for someone.