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

Remove partial support for handling -0000 offset #1411

Merged
merged 3 commits into from
Feb 7, 2024

Conversation

pitdicker
Copy link
Collaborator

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 the Parsed struct if the offset is -0000.
timezone_offset_2822 returns an Option<i32> that should be None to differentiate this case from +0000.

But then timezone_offset_2822 does return Some(0) on the -0000 input. That makes the Option 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 new Error type for 0.5, I just need something consistent to work on.

Copy link

codecov bot commented Feb 4, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (b92b736) 91.84% compared to head (1d1b336) 91.84%.
Report is 19 commits behind head on main.

❗ Current head 1d1b336 differs from pull request most recent head 18d8459. Consider uploading reports for the commit 18d8459 to get more accurate results

Files Patch % Lines
src/format/parse.rs 87.50% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@pitdicker pitdicker force-pushed the remove_unknown_offset branch 3 times, most recently from 1d1b336 to dbf2d27 Compare February 4, 2024 20:29
// 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;
Copy link
Member

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.

Copy link
Collaborator Author

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

Copy link
Member

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.

Copy link
Collaborator Author

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.

@pitdicker pitdicker merged commit 42db00a into chronotope:main Feb 7, 2024
30 of 35 checks passed
@pitdicker pitdicker deleted the remove_unknown_offset branch February 7, 2024 12:36
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.

2 participants