bpo-19376: Added doc. by toanant · Pull Request #10243 · python/cpython (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation33 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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 our records indicate you have not signed the CLA. For legal reasons we need you to sign this 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!

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!

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.

Contributor

taleinat left a comment • Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this again, IMO we should add two notes:

1. Not proving a year leads to using 1900.
2. Feb. 29th without a year will fail due to the above.

These should be added at the end of the "Notes" section at the end of this page. It would probably be best to make this a single note with two short sentences.

I want you to know that seeing your suggested PR has helped me realize how I think this should be done, and hope you accept my requesting multiple revisions as a necessary part of the process.

@taleinat Generally speaking I agree that there should be two things communicated:

1. The default value is 1900-01-01T00:00:00.000 - any components parsed out of a string will replace the corresponding value in that datetime, and when using strftime, any components requested in the format string but missing from the object itself (in the case of datetime.date and datetime.time objects) will be pulled from the default value.
2. Because of point 1, parsing any string with "Feb 29" but no year will cause an error, since 1900 is not a leap year.

There are currently 2 paragraphs in the preamble covering the behavior of strftime with time and date (though they do not mention that they are specific to strftime, which makes it a bit confusing). I would propose that we cover point 1 in 2-3 paragraphs that would replace those existing paragraphs and give a more general view of "what happens when information is missing". Point 2 can be a footnote referenced in those paragraphs.

Unfortunately I don't, at the moment, have a proposed wording for this.

Also, @toanant I would like to echo @taleinat's thanks for your patience with this. I know it can feel like reviewers are being persnickety about minor things (particularly if they disagree with one another), and I hope that we have done a good job communicating to you that your contribution to the project is valuable.

@toanant, I agree with @pablogsal: We should first mention that the default is 1900-01-01T00:00:00.000 and all non-supplied components will fall back to those values, and then as a followup mention that that means that parsing a string with Feb 29 but no year will fail.

@pganssle Thanks for your helpful inline comments, I've applied them accordingly.
Please let me know in case any modification needed for latest patch.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your patience @toanant, this looks good enough to me. Kind of hard to get this across clearly, so maybe there's a better way to do it, but I definitely think this change is a positive change.

After further review, I suggest rewording the first paragraph to the following, which is more concise and hopefully clearer. If others approve, I will make this change and merge this PR.

For the datetime.strptime class method, the default value is 1900-01-01T00:00:00.000: any components not specified in the format string will be pulled from the default value.

Suggested ReST:
For the :meth:`datetime.strptime` class method, the default value is ``1900-01-01T00:00:00.000``: any components not specified in the format string will be pulled from the default value. [#]_

Added doc mentioning datetime.strptime() without a year fails for Feb 29.

@pganssle @taleinat I've incorporated changes suggested by you. Do let me know any change needed for latest patch.

Member

pganssle left a comment • Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, sorry for the long delay in the review.

Thanks @csabella for the ping!

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

May 18, 2019

…fails for Feb 29. (pythonGH-10243)

(cherry picked from commit 56027cc)

Co-authored-by: Abhishek Kumar Singh toanant@users.noreply.github.com

Thank you @toanant for the contribution and thank you @pganssle for the review and approval. Also thanks to @taleinat for the reviews. This has been merged! 🎉

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

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

taleinat pushed a commit to taleinat/cpython that referenced this pull request

May 21, 2019

… year fails for Feb 29. (pythonGH-10243)

(cherry picked from commit 56027cc)

Co-authored-by: Abhishek Kumar Singh toanant@users.noreply.github.com

taleinat added a commit that referenced this pull request

May 21, 2019

… year fails for Feb 29. (GH-10243)

(cherry picked from commit 56027cc)

Co-authored-by: Abhishek Kumar Singh toanant@users.noreply.github.com

@vstinner, shouldn't this also be backported to 3.6 and 2.7?