Added tests, Fixed microseconds in ToSql implementations

[cmarkh]

I took a stab at making some tests as per sfackler#1164 (comment).

Tests:

Added tests in /tokio-postgres/tests/test/types/jiff_01.rs
for civil::Date, civil::DateTime, civil::Time, Timestamp

Fixes:

• Changed Span::new().microseconds(x) to Span::new().try_microseconds(x) in the FromSql implementations. The former panics, the latter returns a Result.

• There was a misuse of Span when trying to get the number of microseconds since the epoch in the ToSql implementations.

For example, the following code:

let epoch = DateTime::constant(2000, 1, 1, 0, 0, 0, 0);
let datetime =  DateTime::strptime("'%Y-%m-%d %H:%M:%S.%f'", "'1970-01-01 00:00:00.010000000’”)?;
datetime.since(epoch)?.get_microseconds()

returns the value of the field microseconds in the span, not the total number of microseconds if you convert all of the days, hours, minutes, etc to microseconds and sum them.

What we want is:

datetime.since(epoch)?.total(Unit::Microsecond)? as i64

Aside:
——
Fwiw @BurntSushi, it’s clear to me now and the docs were helpful once I knew to look, but initially I found this confusing.
This is a common operation so might I suggest including an example of getting the total seconds of a Span, or similar, in the Usage section of the Readme.

I would also have a preference for adding methods to Span for something like .total_microseconds(), .total_seconds(), etc.

Maybe I’m showing my ignorance of standards/common expectations here, but at a glance I might also opt for .set_microseconds(x) over .microseconds(x) just to disambiguate it. If one were to not be prepended, I would actually have defaulted to having the getter be .microseconds() rather than the setter. Though I do see what you’re doing with .try_microseconds(). I personally would probably never use the panic version of the methods but can understand others preferring it. I wonder if people might naively not realize the non-try version panics.

As a final commentary (sorry), I would personally rather have the fields be public and get/set them directly. I see what you’re doing with the min and max ranges (which is nice!) so perhaps that just wouldn’t be clean enough to do. But generally I’d rather do span.microseconds = x, span.microseconds = x.into(), or even span.microseconds = SpanMicroseconds::try_new(x)?, and then be able to get with a simple span.microseconds. Probably my weakest suggestion here given I have a bias against getters and setters unless absolutely necessary. Having set_, get_, and total_ would probably be sufficient to make things very clear for the user.

Separately, a method or other mechanism for checking what type of Error is returned would be helpful. One of my tests uses a very dirty method of

assert!(display_string.contains("is not in the required range of"));

(I’m enjoying the library so far and appreciate the evident thought and work you put into this. Just my 2c for whatever it’s worth while the library is still being developed).
——

[BurntSushi]

Thanks for the comments @cmarkh! I decided to carry them over to BurntSushi/jiff#72 and answered you there to avoid filling up this PR with Jiff API design discussion. :-)

[allan2]

@cmarkh, thank you for the PR. Unfortunately, I only saw it after I had finished all of my tests and changes.

Our implementations are similar, with minor differences like Span::round instead of Span::total and FromStr to parse in tests instead of strptime.

My apologies for missing this and not merging this earlier.