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

fix(prost-types): Allow unknown local time offset #1109

Merged
merged 1 commit into from
Aug 2, 2024

Conversation

caspermeijn
Copy link
Collaborator

chrono allows a negative zero local offset. I think we should follow their example.

The RFC 3339 spec says this is a special value, but doesn't specify how to handle the situation. https://www.rfc-editor.org/rfc/rfc3339#section-4.3

`chrono` allows a negative zero local offset. I think we should follow their example.

The RFC 3339 spec says this is a special value, but doesn't specify how to handle the situation.
https://www.rfc-editor.org/rfc/rfc3339#section-4.3
@caspermeijn
Copy link
Collaborator Author

@mumbleskates I found another difference with chrono. Do you agree that following chrono parsing is the way to go?

@mumbleskates
Copy link
Contributor

I'm less sure about this. There are quite a few total differences between prost-types and chrono, and i don't necessarily hold chrono as a gold standard, just as a known correct implementation of date-and-time math. there are other differences too: chrono also accepts years with different numbers of digits, and represents leap seconds by letting nanos overflow rather than doing anything else.

looking at the standards, it seems that -00:00 does indeed represent "unknown local offset" in rfc3339, and is outright forbidden in iso8601. in this sense (as well as handling unsigned year numbers with other than four digits) i think prost-types seems more correct than chrono, and these cases could easily merit exceptions in the fuzzer rather than adaptation by the library.

@mumbleskates
Copy link
Contributor

welllllll.... it is true that "unknown local offset" in rfc3339 does say that the UTC time is known here. Which does mean that we can use the time; it's equivalent to Zulu, we just don't know what the end user wants for humanized time... but we aren't actually attempting to represent the time zone, we're throwing it away regardless. Maybe this change is completely fine

@caspermeijn caspermeijn added this pull request to the merge queue Aug 2, 2024
Merged via the queue into tokio-rs:master with commit 7e55cda Aug 2, 2024
16 checks passed
@caspermeijn caspermeijn deleted the date_time_negative_offset branch August 2, 2024 13:04
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