bpo-34903: For datetime.datetime.strptime(), the leading zero for some two-digit formats is optional. #14149
Conversation
|
/cc @abalkin |
There was a problem hiding this comment.
These changes look mostly pretty good, I've commented inline with some nitpicks.
One thing I don't want to get lost in the migration from #9747 is that Brett suggested that the tests be moved to test_strptime. I gave my reasoning why I think datetimetester.py in this comment on the original issue, but I would like @brettcannon to weigh in on this before just overriding his suggestion.
Finally, I should note that once the changes are approved, I'd like to run these tests against the buildbots before merging. strptime and strftime are particularly sensitive to weird edge cases on the "long tail" platforms, so changes like this are very likely to need adjustment after they've been run on the buildbots.
Misc/NEWS.d/next/Documentation/2019-06-17-09-36-46.bpo-34903.r_wGRc.rst
Outdated
Show resolved
Hide resolved
|
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 |
|
Paul,
I am ok with your nitpicks except the change to the blurb should have
parens after strptime to avoid Brett's wrath ;-). Also I'm afraid I'm at a
loss as to how to update a blurb.
I agree that using the datetime rather than a string is better.
I used the time_helper subroutine just because it was easier to write it
that way. If you think it's easier to read inline, that's fine—you're the
guy who has to read it.
I'll update and commit the changes to datetimetester.py and maybe will have
figured out how to update the blurb by then.
Regards,
Mike
…On Mon, Jun 17, 2019 at 4:20 PM Paul Ganssle ***@***.***> wrote:
***@***.**** requested changes on this pull request.
These changes look mostly pretty good, I've commented inline with some
nitpicks.
One thing I don't want to get lost in the migration from #9747
<#9747> is that Brett suggested
that the tests be moved to test_strptime. I gave my reasoning why I think
datetimetester.py in this comment on the original issue
<#9747 (comment)>, but
I would like @brettcannon <https://github.com/brettcannon> to weigh in on
this before just overriding his suggestion.
Finally, I should note that once the changes are approved, I'd like to run
these tests against the buildbots *before* merging. strptime and strftime
are particularly sensitive to weird edge cases on the "long tail"
platforms, so changes like this are very likely to need adjustment after
they've been run on the buildbots.
------------------------------
In Misc/NEWS.d/next/Documentation/2019-06-17-09-36-46.[bpo-34903](https://bugs.python.org/issue34903).r_wGRc.rst
<#14149 (comment)>:
> @@ -0,0 +1,3 @@
+For datetime.datetime.strptime(), the leading zero for some two-digit formats is optional.
You are encouraged to add a "Patch by" message to your NEWS entries, to
give you the well-deserved credit for the patch.
And since I am suggesting that you do this anyway, I feel less bad about
bikeshedding on the content of the message: I think it can be reworded to
be a bit more concise:
"Documented that in :meth:datetime.datetime.strptime, the leading zero in
some two-digit formats is optional. Patch by Mike Gleen."
------------------------------
In Lib/test/datetimetester.py
<#14149 (comment)>:
> + '02/03 4am:05:06', '%j/%y %I%p:%M:%S','2003-01-02T04:05:06'),
+ ('%w',
+ '6/04/03', '%w/%U/%y', '2003-02-01T00:00:00'),
+ ('%W', # %u requires a single digit.
+ '6/4/2003', '%u/%W/%Y', '2003-02-01T00:00:00'),
+ ('%V',
+ '6/4/2003', '%u/%V/%G', '2003-01-25T00:00:00'),
+ ]
+ def time_helper(reason, string, format, target):
+ reason = 'test single digit ' + reason
+ with self.subTest(reason=reason,
+ string=string,
+ format=format,
+ target=target):
+ newdate = strptime(string, format)
+ self.assertEqual(newdate.isoformat(timespec='seconds'), target,
Is there a particular reason you are converting the datetime to a string
representation and then comparing that, rather than just comparing the
datetimes directly?
It adds another thing that can fail that is unrelated to what you are
testing here, so I think it would be best avoided unless there's something
about datetime equality semantics that makes comparing the strings a better
test.
Instead, I recommend something like:
dt1 = self.theclass(2003, 1, 2, 4, 5, 6)
dt2 = self.theclass(2003, 2, 1)
inputs = [
('%d', '1/02/03 4:5:6', '%d/%m/%y %H:%M:%S', dt1),
('%m', '01/2/03 4:5:6', '%d/%m/%y %H:%M:%S', dt1),
...
('%V', '6/4/2003', '%u/%V/%G', dt2),
...
Then you can change the assertion to: self.assertEqual(newdate, target,
msg=reason).
------------------------------
In Lib/test/datetimetester.py
<#14149 (comment)>:
> + '6/04/03', '%w/%U/%y', '2003-02-01T00:00:00'),
+ ('%W', # %u requires a single digit.
+ '6/4/2003', '%u/%W/%Y', '2003-02-01T00:00:00'),
+ ('%V',
+ '6/4/2003', '%u/%V/%G', '2003-01-25T00:00:00'),
+ ]
+ def time_helper(reason, string, format, target):
+ reason = 'test single digit ' + reason
+ with self.subTest(reason=reason,
+ string=string,
+ format=format,
+ target=target):
+ newdate = strptime(string, format)
+ self.assertEqual(newdate.isoformat(timespec='seconds'), target,
+ msg=reason)
+ for testcase in inputs:
Using a closure for this seems unusual to me, did you have a particular
reason for doing it this way? I can see for example that it does help to
isolate the individual test cases by creating a new scope for each subtest,
*but* I do find it a bit harder to read.
If you didn't have any particular reason, I'm inclined to say you should
just move the body of time_helper into this for loop. If you have a
strong reason or feel strongly about it, I think you can make the test more
readable by changing the following things:
1.
Move the subtest call out of the function, so that the function body
isn't as deeply nested.
2.
Rename time_helper to something like run_strptime_test_case or
assert_strptime.
Then the loop would be something like this:
for testcase in inputs:
with self.subTest(reason=testcase[0]):
run_strptime_test_case(*testcase)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#14149?email_source=notifications&email_token=AAOL3JVRSGLJGX4IOIYO2PDP26TTXA5CNFSM4HYU2SQ2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB3XQVVQ#pullrequestreview-250546902>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAOL3JUVWT5SVUL6H2B5KR3P26TTXANCNFSM4HYU2SQQ>
.
|
Replace the string used as a target with a datetime instance.
|
I have made the requested changes; please review again. |
|
Thanks for making the requested changes! @pganssle: please review the changes made to this pull request. |
| @@ -0,0 +1 @@ | |||
| Documented that in :meth:datetime.datetime.strptime(), the leading zero in some two-digit formats is optional. Patch by Mike Gleen. | |||
There was a problem hiding this comment.
| Documented that in :meth:datetime.datetime.strptime(), the leading zero in some two-digit formats is optional. Patch by Mike Gleen. | |
| Documented that in :meth:`datetime.datetime.strptime()`, the leading zero in some two-digit formats is optional. Patch by Mike Gleen. |
This needs backticks so Sphinx can turn it into a link to the relevant method.
|
@mikegleen Thanks Mike, these changes look good to me! I made one last edit suggestion inline to fix an issue with the What's New. I'm finding it a bit hard to interpret the CI failures, but it's just the documentation builds that are failing, so it's possible that that's just a linter detecting the missing backticks. |
Add backticks so Sphinx can turn the method name into a link to the relevant method.
|
Paul,
Ok, backticks added.
Rgds,
Mike
…On Mon, Jun 17, 2019 at 7:16 PM Paul Ganssle ***@***.***> wrote:
@mikegleen <https://github.com/mikegleen> Thanks Mike, these changes look
good to me! I made one last edit suggestion inline to fix an issue with the
What's New. I'm finding it a bit hard to interpret the CI failures, but
it's just the documentation builds that are failing, so it's possible that
that's just a linter detecting the missing backticks.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#14149?email_source=notifications&email_token=AAOL3JRPVOFYKG5R5CU43CTP27II7A5CNFSM4HYU2SQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODX4AOVQ#issuecomment-502794070>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAOL3JQ6JEBGXTZXJHPCOE3P27II7ANCNFSM4HYU2SQQ>
.
|
|
No wrath from me. 😉 At this point my review is "whatever @pganssle says". |
There was a problem hiding this comment.
I ran this through the buildbots yesterday and it did not create any new failures. The only semi-related failure was on Alpine Linux, but that is related to an earlier PR, so it seems like this documentation is accurate for all supporting platforms.
Thanks @mikegleen!
|
I have made the requested changes; please review again. Thanks to the core devs and administrators (and bots) who patiently walked me through this really minor fix. I am sure that I've benefited more learning about the dev process than Python will benefit from the actual code change. |
|
Thanks for the review @vstinner. I can merge this when the CI passes unless there are any other objections. Since this is just a documentation fix (with tests!), we can also backport to Python 3.7. |
|
There was a failure in the Mac PR that looked unrelated, seemed like the multiprocessing tests hung for about 56 minutes. Assuming it wasn't just some other kind of weird fluke, I suppose it could be a race condition in the multiprocessing tests, but I don't think it's related to this PR, so I'm going to go ahead and merge. I'll try to dig up the broken log a bit later and see if it's worth reporting. |
|
Thanks @mikegleen for the PR, and @pganssle for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7. |
|
GH-14205 is a backport of this pull request to the 3.7 branch. |
…ythonGH-14149) For datetime.datetime.strptime(), the leading zero for some two-digit formats is optional. This adds a footnote to the strftime/strptime documentation to reflect this fact, and adds some tests to ensure that it is true. bpo-34903 (cherry picked from commit 6b9c204) Co-authored-by: Mike Gleen <mike.gleen@gmail.com>
|
@pganssle This PR should also be backported to 3.8, no? |
…H-14149) For datetime.datetime.strptime(), the leading zero for some two-digit formats is optional. This adds a footnote to the strftime/strptime documentation to reflect this fact, and adds some tests to ensure that it is true. bpo-34903 (cherry picked from commit 6b9c204) Co-authored-by: Mike Gleen <mike.gleen@gmail.com>
|
@ZackerySpytz Oh jeez, I completely forgot that master isn't 3.8 anymore! Thanks! |
|
Thanks @mikegleen for the PR, and @pganssle for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8. |
|
GH-14206 is a backport of this pull request to the 3.8 branch. |
…H-14149) For datetime.datetime.strptime(), the leading zero for some two-digit formats is optional. This adds a footnote to the strftime/strptime documentation to reflect this fact, and adds some tests to ensure that it is true. bpo-34903 (cherry picked from commit 6b9c204) Co-authored-by: Mike Gleen <mike.gleen@gmail.com>
|
The macOS issue has been reported but we have no clue so far:
https://bugs.python.org/issue37245
…--
Night gathers, and now my watch begins. It shall not end until my death.
|
…ythonGH-14149) For datetime.datetime.strptime(), the leading zero for some two-digit formats is optional. This adds a footnote to the strftime/strptime documentation to reflect this fact, and adds some tests to ensure that it is true. bpo-34903
…ythonGH-14149) For datetime.datetime.strptime(), the leading zero for some two-digit formats is optional. This adds a footnote to the strftime/strptime documentation to reflect this fact, and adds some tests to ensure that it is true. bpo-34903
This pull request replaces #9747 which I am closing. Sorry, couldn't figure out how to cleanly update the original.
I am adding tests to datetimetester.py following discussion with Paul Ganssle. Please let me know if this causes a problem.
https://bugs.python.org/issue34903