bpo-43295: Fix error handling of datetime.strptime format string '%z' by noormichael · Pull Request #24627 · python/cpython

noormichael · 2021-02-23T05:08:07Z

Previously, datetime.strptime would match 'z' with the format string '%z' (for UTC offsets), throwing an IndexError by erroneously trying to parse 'z' as a timestamp. As a special case, '%z' matches the string 'Z' which is equivalent to the offset '+00:00', however this behavior is not defined for lowercase 'z'.

This change ensures a ValueError is thrown when encountering the original example, as follows:

>>> from datetime import datetime
>>> datetime.strptime('z', '%z')
ValueError: time data 'z' does not match format '%z'

https://bugs.python.org/issue43295

Automerge-Triggered-By: GH:pganssle

the-knights-who-say-ni · 2021-02-23T05:08:10Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@noormichael

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

pganssle

Thanks for the quick fix on this! Just a few minor changes.

pganssle · 2021-02-26T16:27:46Z

Two things here:

I much prefer the context manager version of assertRaises, since it's much clearer what calls are being tested.

It would be preferable to test this using only the public interface. I realize that there are other uses of _strptime._strptime_time in this file, but going forward I'd like to keep the tests focused on the public interface, so can you try and exercise this using time.strptime and/or datetime.strptime?

pganssle · 2021-02-26T16:33:38Z

This is a very good summary of the issue, but it is long for a news stub. They're usually 1 or two lines like this: https://docs.python.org/3/whatsnew/changelog.html

How about:

Previously, :meth:`datetime.datetime.strptime` would fail with `IndexError` when the string `'z'` was found for the `%z` format specifier. This has been fixed to properly raise `ValueError` instead.

(You'll want to check to make sure that the :meth: directive works correctly, I may have made a mistake with that.
A BPO link is automatically generated, and users who want more information can click on it for an MWE and such.

bedevere-bot · 2021-02-26T16:34:23Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

noormichael · 2021-03-03T04:51:02Z

Is there a way to re-run the tests? test_ssl failed on Ubuntu which has passed on previous runs. In any case, I've made the requested changes. Could you please review this again?

pganssle

@noormichael No need to worry about that. I believe this is something to do with github upgrading the default Ubuntu container from 18.04 to 20.04 and some patch in Ubuntu that breaks OpenSSL. See this bug for more details.

I believe no further action on your part is necessary, this looks good and we can probably wait until that bug is resolved to merge so that backports will go more easily. Thanks for your contribution to CPython!

…%z' (pythonGH-43295)

miss-islington · 2021-03-03T16:58:56Z

@noormichael: Status check is done, and it's a success ✅ .

miss-islington · 2021-03-03T16:59:00Z

Thanks @noormichael for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8, 3.9.
🐍🍒⛏🤖

bedevere-bot · 2021-03-03T16:59:18Z

GH-24728 is a backport of this pull request to the 3.9 branch.

…pythonGH-24627) Previously, `datetime.strptime` would match `'z'` with the format string `'%z'` (for UTC offsets), throwing an `IndexError` by erroneously trying to parse `'z'` as a timestamp. As a special case, `'%z'` matches the string `'Z'` which is equivalent to the offset `'+00:00'`, however this behavior is not defined for lowercase `'z'`. This change ensures a `ValueError` is thrown when encountering the original example, as follows: ``` >>> from datetime import datetime >>> datetime.strptime('z', '%z') ValueError: time data 'z' does not match format '%z' ``` Automerge-Triggered-By: GH:pganssle (cherry picked from commit 04f6fbb) Co-authored-by: Noor Michael <nsmichael31@gmail.com>

bedevere-bot · 2021-03-03T16:59:30Z

GH-24729 is a backport of this pull request to the 3.8 branch.

…pythonGH-24627) Previously, `datetime.strptime` would match `'z'` with the format string `'%z'` (for UTC offsets), throwing an `IndexError` by erroneously trying to parse `'z'` as a timestamp. As a special case, `'%z'` matches the string `'Z'` which is equivalent to the offset `'+00:00'`, however this behavior is not defined for lowercase `'z'`. This change ensures a `ValueError` is thrown when encountering the original example, as follows: ``` >>> from datetime import datetime >>> datetime.strptime('z', '%z') ValueError: time data 'z' does not match format '%z' ``` Automerge-Triggered-By: GH:pganssle (cherry picked from commit 04f6fbb) Co-authored-by: Noor Michael <nsmichael31@gmail.com>

Immorningstar · 2021-03-03T17:01:21Z

Plz don’t mail me I’m not interested in anything

…

Sent from my iPhone

On 03-Mar-2021, at 10:30 PM, Bedevere (bot) ***@***.***> wrote: GH-24729 is a backport of this pull request to the 3.8 branch. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

miss-islington · 2021-04-28T16:23:27Z

Thanks @noormichael for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

bedevere-bot · 2021-04-28T16:23:35Z

GH-25695 is a backport of this pull request to the 3.9 branch.

…pythonGH-24627) Previously, `datetime.strptime` would match `'z'` with the format string `'%z'` (for UTC offsets), throwing an `IndexError` by erroneously trying to parse `'z'` as a timestamp. As a special case, `'%z'` matches the string `'Z'` which is equivalent to the offset `'+00:00'`, however this behavior is not defined for lowercase `'z'`. This change ensures a `ValueError` is thrown when encountering the original example, as follows: ``` >>> from datetime import datetime >>> datetime.strptime('z', '%z') ValueError: time data 'z' does not match format '%z' ``` Automerge-Triggered-By: GH:pganssle (cherry picked from commit 04f6fbb) Co-authored-by: Noor Michael <nsmichael31@gmail.com>

…GH-24627) (#25695) Previously, `datetime.strptime` would match `'z'` with the format string `'%z'` (for UTC offsets), throwing an `IndexError` by erroneously trying to parse `'z'` as a timestamp. As a special case, `'%z'` matches the string `'Z'` which is equivalent to the offset `'+00:00'`, however this behavior is not defined for lowercase `'z'`. This change ensures a `ValueError` is thrown when encountering the original example, as follows: ``` >>> from datetime import datetime >>> datetime.strptime('z', '%z') ValueError: time data 'z' does not match format '%z' ``` Automerge-Triggered-By: GH:pganssle (cherry picked from commit 04f6fbb) Co-authored-by: Noor Michael <nsmichael31@gmail.com> Co-authored-by: Noor Michael <nsmichael31@gmail.com>

noormichael requested review from abalkin and pganssle as code owners February 23, 2021 05:08

the-knights-who-say-ni added the CLA not signed label Feb 23, 2021

bedevere-bot added awaiting review awaiting core review and removed awaiting review labels Feb 23, 2021

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Feb 23, 2021

serverwentdown approved these changes Feb 24, 2021

View reviewed changes

itchyny approved these changes Feb 26, 2021

View reviewed changes

pganssle requested changes Feb 26, 2021

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting core review labels Feb 26, 2021

pganssle approved these changes Mar 3, 2021

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Mar 3, 2021

pganssle added needs backport to 3.8 labels Mar 3, 2021

noormichael added 6 commits March 3, 2021 11:13

bpo-43295: Throw ValueError when datetime.strptime matches 'z' with '…

e999f60

…%z' (pythonGH-43295)

bpo-43295: Fix Documentation (pythonGH-43295)

018b7f9

bpo-43295: Format Source Code in Documentation (pythonGH-43295)

ab99696

bpo-43295: Addressed Feedback (pythonGH-43295)

6ef4ebe

bpo-43295: Shorten Documentation (pythonGH-43295)

2c0eee4

bpo-43295: Fix backquotes (pythonGH-43295)

901c1fe

pganssle force-pushed the fix-issue-43295 branch from 5d47504 to 901c1fe Compare March 3, 2021 16:14

pganssle added the 🤖 automerge label Mar 3, 2021

miss-islington merged commit 04f6fbb into python:master Mar 3, 2021

bedevere-bot removed the awaiting merge label Mar 3, 2021

bedevere-bot removed the needs backport to 3.9 label Mar 3, 2021

bedevere-bot removed the needs backport to 3.8 label Mar 3, 2021

vstinner added the needs backport to 3.9 label Apr 28, 2021

bedevere-bot removed the needs backport to 3.9 label Apr 28, 2021

Uh oh!

Conversation

noormichael commented Feb 23, 2021 • edited by miss-islington Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Feb 23, 2021

CLA Missing

Uh oh!

pganssle left a comment

Choose a reason for hiding this comment

Uh oh!

pganssle Feb 26, 2021

Choose a reason for hiding this comment

Uh oh!

pganssle Feb 26, 2021

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Feb 26, 2021

Uh oh!

noormichael commented Mar 3, 2021

Uh oh!

pganssle left a comment

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Mar 3, 2021

Uh oh!

miss-islington commented Mar 3, 2021

Uh oh!

bedevere-bot commented Mar 3, 2021

Uh oh!

bedevere-bot commented Mar 3, 2021

Uh oh!

Immorningstar commented Mar 3, 2021 via email

Uh oh!

miss-islington commented Apr 28, 2021

Uh oh!

bedevere-bot commented Apr 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

noormichael commented Feb 23, 2021 •

edited by miss-islington

Loading