-
Notifications
You must be signed in to change notification settings - Fork 533
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
Remove partial support for handling -0000
offset
#1411
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1411 +/- ##
==========================================
- Coverage 91.84% 91.84% -0.01%
==========================================
Files 38 38
Lines 17518 17517 -1
==========================================
- Hits 16090 16089 -1
Misses 1428 1428 ☔ View full report in Codecov by Sentry. |
1d1b336
to
dbf2d27
Compare
src/format/parse.rs
Outdated
// This range check is similar to the one in `FixedOffset::east_opt`, so it would be redundant. | ||
// But it is possible to read the offset directly from `Parsed`. We want to only successfully | ||
// populate `Parsed` if the input is fully valid RFC 3339. | ||
const MAX_OFFSET: i32 = 23 * 3600 + 59 * 60; |
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'd like for this to be two commits:
- One for using a constant and the reformulated
Range::contains()
- One for changing the value from 86400 to 863400
I also think 23 * 3600 + 59 * 60
is a surprising way to express this? I think (24 * 60 - 1) * 60 might be clearer.
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 also think
23 * 3600 + 59 * 60
is a surprising way to express this? I think (24 * 60 - 1) * 60 might be clearer.
It comes from the definition in RFC 3339 as copied in comment of this function:
// time-hour = 2DIGIT ; 00-23
// time-numoffset = ("+" / "-") time-hour ":" time-minute
// time-minute = 2DIGIT ; 00-59
// time-offset = "Z" / time-numoffset
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.
Right. (23 * 60 + 59) * 60 could also work, I guess, but I find the previous suggestion more intuitive. I'll leave it up to you.
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.
Done. Added 'Max for the hours field is 23
, and for the minutes field 59
.' as a comment.
dbf2d27
to
b476e7d
Compare
b476e7d
to
18d8459
Compare
RFC 2822 and others have a special encoding to indicate the offset of a datetime is unknown:
-00:00
.We currently have code in
format::parse
that ensures we do not provide an offset to theParsed
struct if the offset is-0000
.timezone_offset_2822
returns anOption<i32>
that should beNone
to differentiate this case from+0000
.But then
timezone_offset_2822
does returnSome(0)
on the-0000
input. That makes theOption
and the code to handle it useless.I would like to see proper support for encoding this value in a
FixedOffset
, as in #1042.But for now the inconsistency just messes up the error variants we return.
Because I am currently working on converting the parsing code from
ParsedError
to a newError
type for 0.5, I just need something consistent to work on.