experience with adding support to rust-postgres

[BurntSushi]

Copied from allan2/rust-postgres#1 by @cmarkh:

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]

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'm not sure this belongs in the README, but certainly at least worth a mention in the crate docs and/or Span docs. There's so much information that it's really hard to know what to prioritize.

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

Can you say more about what this would buy you over total(Unit::Second)?

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.

I talked more about this here: #37

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.

The panic version is appropriate and correct to use when you know the span unit values are valid. For example, Span::new().days(1). Since it's a literal in your program, you want it to panic if it's wrong, since it's a bug. It's exactly analogous to Regex::new("...").unwrap().

It is indeed possible that folks will use the panicking variant when they should, for example, with user input that isn't validated. But the use case of building a Span with a literal in the program source code is also extremely important. So they have to be balanced against one another.

As a final commentary (sorry), I would personally rather have the fields be public and get/set them directly.

This definitely can't happen. Firstly, because of the ranged integers. Those are private implementation details of Jiff used to avoid overflow bugs. Secondly, because Span maintains internal invariants, like normalizing the sign of the unit values. If everything were exposed, the caller would have to be responsible for ensuring that the sign of all values is consistent. Either all positive or all negative. Because the sign applies to the overall Span itself. And a design like SpanMicroseconds::try_new would add a whole mess of types to Jiff that I'm not sure are worth adding.

Just in general, I almost never expose the internals of a struct in the public API of libraries. Because it significantly limits what you can do in a semver compatible release.

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"));

Yeah I intentionally started with a very opaque error type. See #8. I'm generally pretty happy to add some kind of introspection capabilities. Can you say more about why you want to know the specific kind of error?

(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).

<3 Thank you for the feedback!!!

[BurntSushi]

Also, see #21 for my plans to add a SignedDuration type to Jiff which will work more like your "classic" duration that doesn't keep track of each individual unit separately. It's less flexible, but it's what a lot of programmers expect from a "duration" type. Span is a somewhat novel concept that comes from the Temporal project. To head off the bug you found where Span::get_microseconds() doesn't return the total number of microseconds, I did add more docs to the getters indicating as such. Initially, the docs didn't mention this fact. (Which is maybe what they were when you initially wrote the rust-postgres integration.)