search

will / crystal-pg

a postgres driver for crystal
BSD 3-Clause "New" or "Revised" License
462 stars 77 forks source link

Add support for Postgres INTERVAL #212

Closed pascalbetz closed 3 years ago

pascalbetz commented 3 years ago

https://github.com/will/crystal-pg/issues/211

Add support for INTERVAL.

I just started learning Crystal (which so far works well since I'm coming from Ruby) so any feedback is welcome.

PG::Interval allows to convert to Time::Spanand Time::MonthSpan but does not do any roll-over between those spans (does not add days to months).

Some questions: 1) What exception to raise when PG::Interval can not be contained in Time::Span? 2) Naming of the conversion methods? to_span / to_month_span is there anything better? 3) Is Interval#to_spans useful at all?

will commented 3 years ago

Thank you for this. I squashed your commits and rebased them (outside of github so it cant tell the pr has been accepted), and made a couple of changes myself. I think just the default runtime error is good, as is the method names.

I changed around the ignore_overflow idea to let people optionally approximate months to a number of days of their choosing rather than just drop the months altogether. Raising by default is good so it's not losing precision, but this allows people to either self opt-in to something like 30 and get a probably good enough approximation, or give 0 and have the behavior you had in the patch where it just drops months altogether.

I think maybe some future work could add comparison methods, but it's probably best to get a feel for what is needed from people who are using this for real, so that can wait until then.

I'll take a look at your other patch soon :)