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

Add back support for parsing strings with "GMT" offset #202

Closed
djc opened this issue Jan 16, 2020 · 4 comments
Closed

Add back support for parsing strings with "GMT" offset #202

djc opened this issue Jan 16, 2020 · 4 comments
Labels
C-feature-request Category: a new feature (not already implemented)

Comments

@djc
Copy link

djc commented Jan 16, 2020

I'm trying to upgrade the cookie crate to time 0.2. As I understand it the relevant date format specification is https://tools.ietf.org/html/rfc2616#section-3.3.1. However, the current approach of not having %Z causes a bunch of pain for parsing these dates. Specifically, parse() for OffsetDateTime seems to return InsufficientInformation if I just try to parse with "GMT" in the format. So it seems I'd have to do a dance of (a) parsing a PrimitiveDateTime, (b) calling to_offset() on it, and (c) validating that it had the "GMT" or equivalent.

I think the approach of actually having %Z support only for GMT and UTC was probably a better trade-off, especially since that would allow both a numeric offset as well as some of these abbrevations. Otherwise, perhaps OffsetDateTime could maybe provide a parse_with_offset() (or just parse_utc()) method that allows a non-numeric offset literal in the format string while using the provided offset.

@jhpratt
Copy link
Member

jhpratt commented Jan 16, 2020

I'll actually be upgrading the cookie crate myself tomorrow — the time crate needs some prerequisites before moving forward with that. Sergio gave me the go-ahead earlier tonight. Just letting you know so there's no duplicated efforts 🙂

The approach I have taken in the conversion so far is to parse as a PrimitiveDateTime (using "GMT" explicitly), and then add the offset back in. Time v0.1 had its parsing handled identically to C, which is where the odd behavior came from. It did have surprising behavior in many situations, which is why I removed it. It'll be re-introduced when tzdb support is done (it's not yet started). That will go to a ZonedDateTime, as parsing offsets must be fixed (yes, UTC is, but many zones aren't).

@djc
Copy link
Author

djc commented Jan 16, 2020

I actually already did most of the work, see rwf2/cookie-rs#134.

@jhpratt
Copy link
Member

jhpratt commented Jan 16, 2020

Thanks! I will go through and do a thorough review of changes. I was also going to upgrade it to 2018, but that can be a separate PR.

@jhpratt
Copy link
Member

jhpratt commented Jan 17, 2020

Closing in favor of #193, which will provide this and more.

@jhpratt jhpratt closed this as completed Jan 17, 2020
@jhpratt jhpratt added the C-feature-request Category: a new feature (not already implemented) label Jan 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: a new feature (not already implemented)
Projects
None yet
Development

No branches or pull requests

2 participants