bpo-37372: Fix unpickling certain datetime.time objects from Python 2

[UncombedCoconut]

Fix for: https://bugs.python.org/issue37372 "datetime.time unpickling fails in case of python2 pickle with seconds>=24". Thanks!

https://bugs.python.org/issue37372

[the-knights-who-say-ni]

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

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

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

[tirkarthi]

Does this PR change the old tests to validate this fix? I think it's good to have new tests than modifying existing tests to catch regressions.

[UncombedCoconut]

Does this PR change the old tests to validate this fix? I think it's good to have new tests than modifying existing tests to catch regressions.

That's right, thanks for clarifying the preferred way to update tests. Do you have strong views on what the tests should be "about"? A test on the specific bug seems too niche, and the existing tests' structure doesn't invite a new input/assertion pair, but maybe a slightly broader test on the inputs' limits...

[pganssle]

A test on the specific bug seems too niche, and the existing tests' structure doesn't invite a new input/assertion pair, but maybe a slightly broader test on the inputs' limits...

I think it's fine to add a test for the specific bug. It makes it a lot easier to see just from running the test suite what kind of regression you are getting.

Maybe in this case the right thing to do is to parametrize over both expected input and expected output, e.g.:

tests = [
    (b"cdatetime\ntime\n(S'\\x14;\\x10\\x00\\x10\\x00'\ntR.", (20, 59, 16, 64**2)),'
    ...
    (b"cdatetime\ntime\n(S'\\x14;\\x19\\x00\\x10\\x00'\ntR.", (20, 59, 25, 64**2)),'
]

I also think it would be prudent to add a with self.subTest context manager in the test, so that each of the entries can fail independently.

[UncombedCoconut]

A test on the specific bug seems too niche, and the existing tests' structure doesn't invite a new input/assertion pair, but maybe a slightly broader test on the inputs' limits...

I think it's fine to add a test for the specific bug. It makes it a lot easier to see just from running the test suite what kind of regression you are getting.

Maybe in this case the right thing to do is to parametrize over both expected input and expected output, e.g.:

tests = [
    (b"cdatetime\ntime\n(S'\\x14;\\x10\\x00\\x10\\x00'\ntR.", (20, 59, 16, 64**2)),'
    ...
    (b"cdatetime\ntime\n(S'\\x14;\\x19\\x00\\x10\\x00'\ntR.", (20, 59, 25, 64**2)),'
]

I also think it would be prudent to add a with self.subTest context manager in the test, so that each of the entries can fail independently.

Good idea, thanks. I'll give it a go.

[serhiy-storchaka]

Good catch! The PR LGTM, but please add more details in a news entry and add your credits.

[serhiy-storchaka]

Please add a little more details: "with minutes or seconds >= 24".

Add also "Patch by Justin Blanchard." here and add your name in Misc/ACKS.

[pganssle]

I don't see any more objections (though I don't understand the C part of the code so I'm not going to submit a formal review). @serhiy-storchaka feel free to merge if you were waiting on me.

[UncombedCoconut]

I don't see any more objections (though I don't understand the C part of the code so I'm not going to submit a formal review). @serhiy-storchaka feel free to merge if you were waiting on me.

Thank you very much for your help!

[ned-deily]

@serhiy-storchaka, @pganssle This PR seems to have stalled. Are you planning to merge it?

[miss-islington]

Thanks @UncombedCoconut for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

[miss-islington]

Thanks @UncombedCoconut for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

[bedevere-bot]

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

[bedevere-bot]

GH-15584 is a backport of this pull request to the 3.7 branch.