qnighy / yasna.rs

ASN.1 library for Rust
Apache License 2.0
42 stars 31 forks source link

Switch from `chrono` to `time` #61

Closed connec closed 2 years ago

connec commented 2 years ago

I'm interested in seeing https://github.com/est31/rcgen/issues/65 resolved, so thought I'd take a crack at replacing chrono with time here to see if there was appetite to merge such a change.

Overall the change was fairly mechanical, with times API actually being a bit neater in the end, imo. The main limitation seems to be regarding leap seconds. For now I've gone for simply ignoring them. Another option would be to record it in the UTCTime/GeneralizedTime structs themselves, so that it can be round-tripped in parsing/formatting (but would still not be recorded in the underlying OffsetDateTime, exposed through the datetime() method).

Since chrono types were used in the API, this is a breaking API change. Since a breaking change was already necessary, I also changed the feature to time.

I've tried to preserve the existing formatting, however some of it may have drifted to default rustfmt style. Let me know if anything needs changed.

connec commented 2 years ago

So it looks like time ≥ 3 is not compatible with Rust 1.36.0. Furthermore time has quite an aggressive policy regarding MSRV:

The time crate is guaranteed to compile with any release of rustc from the past six months. Optional feature flags that enable interoperability with third-party crates (e.g. rand) follow the policy of that crate if stricter.

Additionally, one of the change notes says:

Per the policy in the README, this may be bumped within the 0.3 series without being a breaking change.

The current MSRV for the latest time (0.3.4) appears to be 1.51. For the 0.3 line the lowest MSRV is 1.48. It looks like time ≥ 0.2.23 is also advisory free, and it seems that would let the MSRV drop to 1.32.0.

I don't see any documentation for MSRV for this repo.

The two sensible options seem to be:

connec commented 2 years ago

I went ahead and added a commit taking the second approach (caveat the documented MSRV with "Optional feature flags that enable interoperability with third-party crates (e.g. time) follow the policy of that crate if stricter"), and updated the CI workflow to not test with features on MSRV.

Let me know if you'd prefer another approach.

connec commented 2 years ago

Thanks for the feedback!

I'll see what can be done regarding leap second handling. I didn't find much when searching the time repo for leap seconds but hopefully something will be possible.

Good catch about the docs, I should review them more carefully.

Regarding the MSRV, I think that makes sense. I'm inclined to leave it as-is in this PR, sadly not sure I'm able to go down the route of a more general upgrade.

est31 commented 2 years ago

Re the leap seconds, I've opened an issue at: https://github.com/time-rs/time/issues/377 . It seems that the time maintainers want proper handling instead of the improper one that the chrono crate has.

connec commented 2 years ago

I've pushed a couple of commits, one fixing the docs and another with a hack for round-tripping leap seconds. The hack brings back the previous test assertion, and I updated the documentation for GeneralizedTime::datetime to indicate that leap seconds are discarded in the returned OffsetDateTime.

If you'd prefer I can drop the hack and just document as TODO – I started with this before I noticed your last comment 😄