search

arenadotio / pgx

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

Add Pgx_value_ptime with ptime converters #114

Closed jsthomas closed 3 years ago

jsthomas commented 3 years ago

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.

As I mentioned in the issue, I'm new to both this code base and OCaml in general, so please let me know if any of the code here isn't idiomatic and/or doesn't solve the problem the way you had in mind.

jsthomas commented 3 years ago

This is failing in Circle with the error:

[ERROR] No solution for pgx & pgx_async & pgx_lwt & pgx_lwt_mirage & pgx_lwt_unix & pgx_unix & pgx_value_core & pgx_value_ptime: The following dependencies couldn't be met:
          - pgx_value_ptime -> ptime >= v0.8.3
              no matching version

I'm not sure why this is, opam seems to show Ptime 0.8.5 as available.

brendanlong commented 3 years ago

I wonder if our build script is missing an opam update or something. I'll take a look.

brendanlong commented 3 years ago

Looks like we needed 0.8.3 instead of v0.8.3. I wonder if the opam maintainers would be interested in adding a warning for when this happens.

jsthomas commented 3 years ago

Oops, thanks for figuring that out; I should've looked more closely and noticed the missing v.

brendanlong commented 3 years ago

No worries :)

brendanlong commented 3 years ago

Oh I just noticed your comment asked for feedback on the code. I think it looked pretty good to me. One part I'm wondering about is if we could avoid the regexes somehow by using functions built into ptime, but I took a quick look and didn't see any. We might want to consider some performance optimizations around it, like pre-compiling the regexes or trying the regexes in sequence instead of in parallel (so we won't need to run all of them in most cases), but we can do improvements to the internals like that in separate commits if/when the performance matters.

Overall, I thought the PR looked great.

jsthomas commented 3 years ago

Thanks for reviewing! This was a nice issue for learning more about how to use Ptime and about how postgres handles/presents date-times. The "good first issue" and "help wanted" labels were also helpful for finding something to work on.