-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Ensure time parser raises if timezone is missing #5382
Ensure time parser raises if timezone is missing #5382
Conversation
From your commit message it's very hard to tell if you mean
or
Please use the latter style for commit messages. If it helps, imagine telling someone what to do. |
8b56537
to
49b2280
Compare
Fixed. Thanks @RX14 Now this has brought forward an additional issue: The formats by The implementation of A proper overhaul is proposed in #5123 which implements a customized parser for http date strings in |
1660717
to
13d3f58
Compare
13d3f58
to
44bc854
Compare
@straight-shoota what is the status of this PR? |
It's waiting for a review. |
I don't see the downsize in |
I'm not sure about it. IMHO it doesn't make sense to change the semantics of date format parsing just because HTTP dates use #5123 has a proper implementation of date parses for HTTP and other protocol purposes which will eliminate the need for using |
|
No, it parses a time zone offset. Time zone abbreviations in general are ambiguous. |
Closed in favor of #5123 |
Somehow the reopen button is disabled. I understood from the rights to edit the PR you were ok to jump in (and you did already lots) but i will don’t do it again on your PRs. Please creat another PR. |
Maybe because I deleted the branch in between? dunno. I allow maintainers to edit my PRs, but I see this mostly as a means to step in if the original author doesn't respond, needs help or have the time to address changes. Apart from such situations, I don't think it is really useful to edit someone else's PRs without coordination. You should now I'm not offended or anything. I just think it's not very effective. |
Reviewing the changes from #5317 I noticed that the time parser does not raise if it expects a time zone but there is none. Until now it only raised if the format was detected as invalid, but not if it was entirely missing.
There might be similar issues in the time parser, but most other fields are at least somewhat tested while
%z
is not (#5324 will add some tests for%z
)./cc @bcardiff