Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ISO 8601 parsing/formatting for RelativeDuration #14

Merged
merged 14 commits into from
Apr 24, 2024

Conversation

intarga
Copy link
Contributor

@intarga intarga commented Apr 9, 2024

Resolves #13

@intarga
Copy link
Contributor Author

intarga commented Apr 10, 2024

@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.

@intarga intarga marked this pull request as ready for review April 10, 2024 14:42
Copy link
Owner

@olliemath olliemath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good. Aside from some minor comments, the main issue is how to handle nanoseconds, which I really think we should

src/relative_duration/parse.rs Outdated Show resolved Hide resolved
src/relative_duration/parse.rs Outdated Show resolved Hide resolved
src/relative_duration/parse.rs Outdated Show resolved Hide resolved
src/relative_duration/parse.rs Outdated Show resolved Hide resolved
src/relative_duration/parse.rs Show resolved Hide resolved
@olliemath
Copy link
Owner

olliemath commented Apr 15, 2024

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
Copy link
Owner

olliemath commented Apr 15, 2024

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
Copy link
Contributor Author

intarga commented Apr 21, 2024

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

@intarga intarga requested a review from olliemath April 21, 2024 19:15
@olliemath
Copy link
Owner

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)

@olliemath olliemath merged commit d82c847 into olliemath:main Apr 24, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ISO 8601 parser. Wanted feature?
2 participants