search

olliemath / chronoutil

ChronoUtil module provides powerful extensions to rust's Chrono crate.
MIT License
21 stars 9 forks source link

ISO 8601 parsing/formatting for RelativeDuration #14

Closed intarga closed 2 months ago

intarga commented 3 months ago

Resolves #13

intarga commented 3 months ago

@olliemath I think this is good to go. A couple of notes though:

  1. There are a lot of compiler warnings unrelated to this PR about use of deprecated functions/types from chrono, which I guess will cause CI to fail. Do you want me to address these? It seems like the uses of Date are intentional and probably just need an annotation to suppress the warning, as for the uses of from_ymd() it seems like we should switch to from_ymd_opt().
  2. This seems like a good candidate for prop testing, but I guessed you probably don't want to add a dependency for that, let me know if I'm wrong.
olliemath commented 3 months ago

Thanks so much for this.

For 1, yes this currently requires suppressing the warning, however I'm planning to remove support for the deprecated chrono types and functions in 0.3 (can't do this in a minor release).

For 2, property based testing is nice in small amounts, and I'm not adverse to adding another dev dependency. Apart from checking that the parse and format functions don't throw weird errors with arbitrary inputs, we would ideally have a test along the lines of:

assert_eq!(d, RelativeDuration::from_iso_8601(d.to_iso_8601()))

That's the easy direction because equality between RelativeDurations is stable under transformations like Y1M1 -> M13. The harder direction is a check along the lines of

assert_eq!(s, RelativeDuration::from_iso_8601(s).to_iso_8601())

because this will only hold for strings in "reduced" form, so you need a smart generation strategy to generate examples in this form.

olliemath commented 3 months ago

Would it be possible to add some benchmarks for cargo bench (see the benches directory for an example). This way we can work on optimisations in the future.

intarga commented 3 months ago

Thanks for the thorough review! I added property tests and benchmarks as requested.

olliemath commented 2 months ago

Thanks for adding the tests and benchmarks - this looks good - will merge and prepare for the 0.2.7 release.

I plan make one small tweak to rename of the functions from iso_8601 to iso8601 for greater harmony with chrono's parse function names (e.g. parse_from_rfc3339)