-
-
Notifications
You must be signed in to change notification settings - Fork 30.8k
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
bpo-19376: Added doc. #10243
bpo-19376: Added doc. #10243
Conversation
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 |
There was a problem hiding this comment.
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:
- Not proving a year leads to using 1900.
- 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:
There are currently 2 paragraphs in the preamble covering the behavior of 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. |
@taleinat Added Notes as suggested. |
@toanant, I agree with @pablogsal: We should first mention that the default is |
2d527a4
to
02faf30
Compare
@pganssle Thanks for your helpful inline comments, I've applied them accordingly. |
There was a problem hiding this comment.
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 Suggested ReST: |
Added doc mentioning `datetime.strptime()` without a year fails for Feb 29.
There was a problem hiding this comment.
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!
…fails for Feb 29. (pythonGH-10243) (cherry picked from commit 56027cc) Co-authored-by: Abhishek Kumar Singh <toanant@users.noreply.github.com>
@csabella: It may be worth it to backport this doc change to 3.7, but @miss-islington looks sick :-( |
… year fails for Feb 29. (pythonGH-10243) (cherry picked from commit 56027cc) Co-authored-by: Abhishek Kumar Singh <toanant@users.noreply.github.com>
GH-13470 is a backport of this pull request to the 3.7 branch. |
@vstinner, shouldn't this also be backported to 3.6 and 2.7? |
I became lazy and I stopped to backport doc enhancements to Python 2.7. Sorry Python 2.7 :-( Python 3.6 no longer accept doc changes: https://devguide.python.org/#status-of-python-branches |
https://bugs.python.org/issue19376