Seeking feedback before release

[sbarral]

This crate is a slightly improved, epoch-independent version of the MonotonicTime timestamp from the asynchronix crate v0.3.
At the moment it also adds support for no-std, serde and for the chrono crate. Feature-gated support for other time-related crates (e.g. time or hifitime) and serialization crates (e.g. rkyv) could be added later.

But overall, the idea is to keep this crate lean and mean, so it does not intend to become much more than it is today. Also, since it is meant to become a public dependency of asynchronix and replace the current MonotonicTime, we will need to be fairly conservative regarding new additions and breaking changes once it is released.

Any criticism welcome.

EDIT:
I am mostly happy with the design, but am a bit torn about error reporting. Rust's std::time is not itself fully consistent and returns mostly None on out-of-range and overflow (all the checked_* methods) but SystemTime::duration_since returns an Error in such case. I am not sure where to draw the line between what warrants an Option and what warrants an Error.

Even if an Error was returned from the from_* and as_* methods (or maybe these should be try_from_* and try_as_*?), I would prefer to keep it simple and use a trivial type with no value. I find the Duration field in std::time::SystemTimeError to be slightly over-engineered, and given that overflow and out-of-range errors can occur in many ways in TaiTime conversion methods, the Duration field is probably not going to work for us.

[robamu]

This looks nice! I will try to have a look tomorrow :)

[sbarral]

Thanks, be sure to check the newest commit, I have made quite a few changes over the WE...

After being on the fence for a long time, I have finally decided to return errors rather than options except for the checked_* methods. My rationale is that even if at the moment we only return OutOfRangeError, in the future we may add other from_* and to_* methods that will require non-trivial errors to be returned (for instance a parsing error if we have a from_str() method), so returning errors everywhere will help keep the API consistent.

[robamu]

I really like what you did with the new auto_doc_cfg feature, I will add that to my crates as well :)

If more returnvalues are added in the future, maybe it might be a good idea to already use an enum for that, even if it only has one entry? The enum can be marked non_exhaustive to even prevent breaking changes when a new error variant is added.

What do you think about adding From and TryFrom implementations for the various conversion types? The existing methods can still be kept, the additional trait implementations just add a bit of ergonomics and re-use the existing methods.

What do you think about adding Sub/ Add impls for the reference of the time?
For example:

impl<const EPOCH_REF: i64> Add<Duration> for &TaiTime<EPOCH_REF> {
    type Output = TaiTime<EPOCH_REF>;

    fn add(self, other: Duration) -> Self::Output {
        *self + other
    }
}

This just add a little bit of ergonomics to use common operations with just references.

This library could be used on smaller systems now, so it might be useful to add a CI test, it is really easy to forget some feature flag when adding new features during development, and not checking the no_std build again. I did this in my github CI here by testing the thumbv7em-none-eabihf and armv7-unknown-linux-gnueabihf. The second one is not strictly necessary, but that kind of emulates the build process for a common STM32 MCU and a Raspberry Pi.

I have not really used the library yet, so maybe I will only find some issue once I use it. I don't know what your plans for the release were, but it might be useful to use this library a bit on a pre-v1 version before comitting to a stable 1.0 release.

[sbarral]

Yes, auto_doc_cfg cuts a lot of annotation noise, I like it too...

About the enum, I guess the implicit idea behind your suggestion is to use a single error type for all fallible methods? I usually favor specific errors whenever possible so that the user does not need to wonder which enum variants can be ignored when processing the error. What would be the advantage of a single error type?

Regarding TryFrom, in fact I would have used it exclusively if I could, but the problem is that almost all those methods need the additional leap_secs argument. We could use tuples I guess, like (&date_time, 37).try_into() to convert a &chrono::DateTime into a TaiTime, but this does not look very idiomatic and, more importantly, it makes it difficult to understand what are the arguments. The only conversion method that doesn't need the leap seconds argument is to_tai_time(), but sadly this one cannot be replaced by a TryFrom implementations, because impl<E1, E2> TryFrom<E1> for TaiTime<E2> overlaps/conflicts with the default impl<T> TryFrom<T> for T.

[robamu]

Ohh thats right, TryFrom and From can't work with additional arguments, forgot about that. If it can't be used idiomatically, I would not add it either.

You're right about the specific errors.. I am used to unify errors for high-level trait methods and for error propagation, but for such a low-level library this is probably the better way.. I might have a look at my own libraries to check whether error handling can be improved.

[sbarral]

Regarding Sub/Add on reference types, I had a look at std::time::Instant, std::time::System and chrono::DateTime, but neither of them has something like this. My guess is that there is no expectation in general to have these on Copy types since you can always dereference them without any penalty.

The CI check for other target platforms is a good idea, I will add it.

[sbarral]

Closing this issue, v0.1.0 was just published, thanks for the feedback! We can still iron things out for a month or two while asynchronix 0.3.0 is still in the making.