Skip to content

gh-84823: Improve doctrings for datetime parsing methods #20677

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

edison12a
Copy link
Contributor

@edison12a edison12a commented Jun 6, 2020

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.

I have a few comments, but the main thing missing here is also updating the doc-strings of the C versions of these methods.

Lib/datetime.py Outdated
@@ -925,7 +925,33 @@ def ctime(self):
self._day, self._year)

def strftime(self, fmt):
"Format using strftime()."
"""strftime(fmt) -> date_string
Copy link
Contributor

Choose a reason for hiding this comment

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

The "input -> output" style of doc-strings is only used in functions and class-methods in this module, but not for normal (instance) methods. I don't see that it adds anything in this case (and the others changed by this PR).

Lib/datetime.py Outdated
Format codes referring to hours, minutes or seconds will see 0 values.

Args:
format: representation of date_string using format codes.
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. The argument is named "fmt".
  2. The "Args:" style is different from that currently used in these classes, and adds little in this case with just one argument.
  3. I'm not sure "representation of date_string using format codes" would be clear to someone who hadn't seen this before. It seems to me that an example would be very helpful here.

Lib/datetime.py Outdated
Comment on lines 952 to 953
Returns:
date_string: String representation of the date
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO these final two lines don't actually add any information.

Lib/datetime.py Outdated
Comment on lines 936 to 950
Commonly used format codes:
%Y Year with century as a decimal number.
%m Month as a decimal number [01,12].
%d Day of the month as a decimal number [01,31].
%H Hour (24-hour clock) as a decimal number [00,23].
%M Minute as a decimal number [00,59].
%S Second as a decimal number [00,61].
%z Time zone offset from UTC.
%a Locale's abbreviated weekday name.
%A Locale's full weekday name.
%b Locale's abbreviated month name.
%B Locale's full month name.
%c Locale's appropriate date and time representation.
%I Hour (12-hour clock) as a decimal number [01,12].
%p Locale's equivalent of either AM or PM.
Copy link
Contributor

Choose a reason for hiding this comment

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

Copied from time.strftime(), I see. This is very nice!

Perhaps here we should omit the codes referring to hours, minutes and seconds?

Also, where would one find more detailed info and the full list of format codes available? Perhaps consider a reference as to where more detailed information may be found, though I'm not sure what that should be. For time.strftime() we have: "Other codes may be available on your platform. See documentation for the C library strftime function."

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

@taleinat taleinat added docs Documentation in the Doc dir skip news labels Sep 25, 2020
@taleinat
Copy link
Contributor

Also, since the datetime class currently inherits the strftime() method from the date class, it might be worth adding datetime.strftime() which simply calls the parent class's method, just to override the misleading doc-string.

And I forgot to mention, thanks for working on this, it certainly would have been helpful to me many times, and I'm sure for millions of others as well!

@pganssle
Copy link
Member

Also, since the datetime class currently inherits the strftime() method from the date class, it might be worth adding datetime.strftime() which simply calls the parent class's method, just to override the misleading doc-string.

I wish there were a way to do this without the additional unnecessary stack frame, but I can't find a good one. ☹

@taleinat
Copy link
Contributor

taleinat commented Oct 21, 2020

Also, since the datetime class currently inherits the strftime() method from the date class, it might be worth adding datetime.strftime() which simply calls the parent class's method, just to override the misleading doc-string.

I wish there were a way to do this without the additional unnecessary stack frame, but I can't find a good one. frowning_face

I was thinking the exact same thing! But I can't think of a way that wouldn't be a messy hack.

Maybe making __doc__ a property/descriptor?

@taleinat
Copy link
Contributor

Maybe making __doc__ a property/descriptor?

Ack, that doesn't seem to be supported on functions/methods :(

@taleinat
Copy link
Contributor

I wish there were a way to do this without the additional unnecessary stack frame, but I can't find a good one. frowning_face

Looking at this again, the .strftime() implementation is far from optimized, and the overhead of another method call would be negligible. I'm +1 for wrapping it for better doc-strings.

@taleinat
Copy link
Contributor

However, for the datetime class from the _datetime module, which is rather optimized and most often the one used, the doc-string can be overridden without adding an extra function call, by explicitly adding the function to datetime_methods[] with a custom doc-string.

@taleinat
Copy link
Contributor

@SimiCode, I realize that several months have passed since you created this PR. Would you be interested and able to follow this up?

@taleinat
Copy link
Contributor

taleinat commented Jan 20, 2022

Since it's been over a year and @SimiCode hasn't responded, I've gone ahead and made the changes myself.

I even found a simple way to to get a nice doc-string for datetime.strftime() without an extra stack frame :)

@pganssle, it would be great to get your 👀 on this!

Copy link
Contributor

@MaxwellDupre MaxwellDupre left a comment

Choose a reason for hiding this comment

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

Ran 960 tests in 10.773s
OK (skipped=28)
Looks ok.

@ghost
Copy link

ghost commented May 19, 2022

All commit authors signed the Contributor License Agreement.
CLA signed

@AlexWaygood AlexWaygood changed the title bpo-40643: Improve doctrings for datetime parsing methods gh-84823: Improve doctrings for datetime parsing methods Oct 30, 2022
@AlexWaygood AlexWaygood removed the docs Documentation in the Doc dir label Oct 30, 2022
Format using strftime().

Example: "%d/%m/%Y, %H:%M:%S"
"""Convert to a string in the given format via time.strftime().
Copy link
Member

Choose a reason for hiding this comment

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

I think these aren't supposed to be time.strftime(), I think we want to directly reference :manpage:`strftime(3)`. Or at least strftime(3) (I guess these docstrings aren't destined for Sphinx, so probably just strftime(3) is most appropriate)

@@ -3591,7 +3591,21 @@ static PyMethodDef date_methods[] = {
PyDoc_STR("Return ctime() style string.")},

{"strftime", _PyCFunction_CAST(date_strftime), METH_VARARGS | METH_KEYWORDS,
PyDoc_STR("format -> strftime() style string.")},
PyDoc_STR(
"Convert to a string in the given format via time.strftime().\n\n"
Copy link
Member

Choose a reason for hiding this comment

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

Same here as well.

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

@python-cla-bot
Copy link

python-cla-bot bot commented Apr 18, 2025

All commit authors signed the Contributor License Agreement.

CLA signed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants