Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Pgx_value_ptime with ptime converters #97

Closed
brendanlong opened this issue May 12, 2020 · 3 comments · Fixed by #114
Closed

Add Pgx_value_ptime with ptime converters #97

brendanlong opened this issue May 12, 2020 · 3 comments · Fixed by #114

Comments

@brendanlong
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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 brendanlong linked a pull request Jun 21, 2021 that will close this issue
brendanlong pushed a commit that referenced this issue Jun 21, 2021
This change addresses issue #97 by introducing a new package called Pgx_value_ptime. This package is analogous to Pgx_value_core, but uses Ptime for date/date-time processing instead of Core.

The tests for Pgx_value_ptime are pretty much the same as those for Pgx_value_core, although I did add some extra tests for dates.
@brendanlong
Copy link
Contributor Author

@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.

yomimono pushed a commit to yomimono/pgx that referenced this issue Mar 8, 2022
This change addresses issue arenadotio#97 by introducing a new package called Pgx_value_ptime. This package is analogous to Pgx_value_core, but uses Ptime for date/date-time processing instead of Core.

The tests for Pgx_value_ptime are pretty much the same as those for Pgx_value_core, although I did add some extra tests for dates.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants