-
Notifications
You must be signed in to change notification settings - Fork 517
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
Parsed
fixes and documentation
#1439
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1439 +/- ##
==========================================
+ Coverage 92.11% 92.14% +0.03%
==========================================
Files 40 40
Lines 18026 18079 +53
==========================================
+ Hits 16604 16659 +55
+ Misses 1422 1420 -2 ☔ View full report in Codecov by Sentry. |
26e5fc3
to
755fe4f
Compare
Found more oddity to document for the
Actually that whole method is just weird to work with. The logic should be inverted to return a |
755fe4f
to
8b66a78
Compare
I have to admit I read the current code wrong. It is working exactly like it should. The timestamp is assumed to be in UTC, it adds an offset, and then checks the rest of the values as local date and time. Dropped the first commit. |
8b66a78
to
2d99fce
Compare
4d6aedf
to
cadf339
Compare
Found one more missing error case in |
/// | ||
/// - `to_*` methods try to make a concrete date and time value out of set fields. | ||
/// It fully checks any remaining out-of-range conditions and inconsistent/impossible fields. | ||
/// They fully check that all fields are consistent and whether the date/datetime exists. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side thought: arguably the to_()
methods should become impl TryFrom<Parsed> for _
impls?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a few cases they can. Good to keep in mind! I'll come back to it later (before 0.5).
src/format/parsed.rs
Outdated
/// # Resolving algorithm | ||
/// | ||
/// Resolving date/time parts is littered with lots of corner cases, which is why common date/time | ||
/// parsers do not correctly implement it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"do not correctly implement it" -> "do not implement it correctly" is a better word order here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right.
src/format/parsed.rs
Outdated
/// Tries to set the [`year`](#structfield.year) field from given value. | ||
/// Set the 'year' field to the given value. | ||
/// | ||
/// The value can be negative unlike the 'year divided by 100' and 'year modulo 100' fields. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need commas before "unlike". When referencing values stored in other fields, would suggest referencing the field directly, like year_div_100
or even [`Parsed::year_div_100`]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When referencing values stored in other fields, would suggest referencing the field directly, like
year_div_100
or even[`Parsed::year_div_100`]
?
I choose to avoid referencing the fields directly so it is easier to make them private (my proposal for the 0.5.x branch).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, suggest making them #[doc(hidden)]
on this branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I was think the same, I'll do it with a PR that adds the range checks and setter methods.
/// May return `OUT_OF_RANGE` if `value` is negative or if it is greater than 99. | ||
/// Currently only checks the value is positive and not out of range for an `i32`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this sounds pretty weird. "May return blah" but actually "Currently" doesn't? I think we should describe what it does, and not what it doesn't do. So maybe replace "Currently only checks [..]" with "For performance reasons, precise range checks may be deferred in favor of checking on conversion into a target type."?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The part about performance has never really been true or benchmarked. See #1418 (comment).
With incomplete range checks (current):
bench_datetime_parse_from_rfc2822
time: [156.24 ns 157.01 ns 157.85 ns]
Found 7 outliers among 100 measurements (7.00%)
2 (2.00%) high mild
5 (5.00%) high severe
bench_datetime_parse_from_rfc3339
time: [104.25 ns 104.54 ns 104.86 ns]
Found 8 outliers among 100 measurements (8.00%)
2 (2.00%) high mild
6 (6.00%) high severe
With complete range checks:
bench_datetime_parse_from_rfc2822
time: [156.47 ns 156.81 ns 157.16 ns]
change: [-0.3688% +0.2339% +0.7850%] (p = 0.44 > 0.05)
No change in performance detected.
Found 7 outliers among 100 measurements (7.00%)
2 (2.00%) high mild
5 (5.00%) high severe
bench_datetime_parse_from_rfc3339
time: [102.69 ns 103.07 ns 103.49 ns]
change: [-2.4267% -1.7659% -1.1091%] (p = 0.00 < 0.05)
Performance has improved.
Found 8 outliers among 100 measurements (8.00%)
4 (4.00%) high mild
4 (4.00%) high severe
So basically identical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this sounds pretty weird. "May return blah" but actually "Currently" doesn't? I think we should describe what it does, and not what it doesn't do.
My hope was to open a PR to add the complete range checks (instead of the incomplete current ones) as soon as this one is merged and remove all those lines again. Does that sound okay or should I polish this a bit more?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I guess that's fine.
This is shaping up nicely! |
cadf339
to
3ea3af6
Compare
Wow, apparently I found and fixed it last year also in #1042 😆. |
This is the first part of #1418 and applies to the branch for 0.4.x.
We have two small bugs in our implementation of
Parsed
:OUT_OF_RANGE
if the year value didn't match with the values ofyear_div_100
oryear_mod_100
. It should returnIMPOSSIBLE
instead.I extended the documentation of
Parsed
to describe why it exists (the resolution algorithm) and to give an example (fixes #55). Partly taken from the blog post before chrono 0.2: https://lifthrasiir.github.io/rustlog/worklog-2015-02-19.htmlAll the methods now have more accurate documentation that describes their error causes.
The documentation got a couple of rounds of self-review.