Skip to content
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

Merged
merged 1 commit into from
May 18, 2019
Merged

bpo-19376: Added doc. #10243

merged 1 commit into from
May 18, 2019

Conversation

toanant
Copy link
Contributor

@toanant toanant commented Oct 30, 2018

[bpo-19376](https://bugs.python.org/issue19376): Added note mentioning `datetime.strptime()` without a year fails on Feb 29.

https://bugs.python.org/issue19376

@the-knights-who-say-ni
Copy link

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!

@bedevere-bot bedevere-bot added docs Documentation in the Doc dir awaiting review labels Oct 30, 2018
Doc/library/datetime.rst Outdated Show resolved Hide resolved
@the-knights-who-say-ni
Copy link

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!

Doc/library/datetime.rst Outdated Show resolved Hide resolved
@bedevere-bot
Copy link

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.

Copy link
Contributor

@taleinat taleinat left a 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:

  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.

@pganssle
Copy link
Member

pganssle commented Nov 5, 2018

@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.

@pganssle
Copy link
Member

pganssle commented Nov 5, 2018

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
Copy link
Contributor Author

toanant commented Nov 13, 2018

@taleinat Added Notes as suggested.

@taleinat
Copy link
Contributor

@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.

@toanant toanant force-pushed the patch-3 branch 2 times, most recently from 2d527a4 to 02faf30 Compare November 13, 2018 16:54
@toanant
Copy link
Contributor Author

toanant commented Feb 16, 2019

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

Doc/library/datetime.rst Outdated Show resolved Hide resolved
Copy link
Member

@pganssle pganssle left a 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.

@taleinat
Copy link
Contributor

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.
@toanant
Copy link
Contributor Author

toanant commented Feb 17, 2019

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

@csabella csabella requested a review from taleinat May 17, 2019 23:24
@csabella
Copy link
Contributor

@pganssle and @taleinat, please review again after the last changes by @toanant. Thanks!

Copy link
Member

@pganssle pganssle left a 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!

@csabella csabella merged commit 56027cc into python:master May 18, 2019
@miss-islington
Copy link
Contributor

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>
@csabella
Copy link
Contributor

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! 🎉

@miss-islington
Copy link
Contributor

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

@miss-islington
Copy link
Contributor

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

@vstinner
Copy link
Member

@csabella: It may be worth it to backport this doc change to 3.7, but @miss-islington looks sick :-(

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>
@bedevere-bot
Copy link

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

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>
@taleinat
Copy link
Contributor

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

@vstinner
Copy link
Member

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants