gh-97517: Improve doctrings for datetime parsing methods#20677
gh-97517: Improve doctrings for datetime parsing methods#20677edison12a wants to merge 5 commits intopython:mainfrom
Conversation
taleinat
left a comment
There was a problem hiding this comment.
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
|
|
||
| def strftime(self, fmt): | ||
| "Format using strftime()." | ||
| """strftime(fmt) -> date_string |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
- The argument is named "fmt".
- The "Args:" style is different from that currently used in these classes, and adds little in this case with just one argument.
- 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
| Returns: | ||
| date_string: String representation of the date |
There was a problem hiding this comment.
IMHO these final two lines don't actually add any information.
Lib/datetime.py
Outdated
| 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. |
There was a problem hiding this comment.
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."
|
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 |
|
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! |
I wish there were a way to do this without the additional unnecessary stack frame, but I can't find a good one. ☹ |
I was thinking the exact same thing! But I can't think of a way that wouldn't be a messy hack. Maybe making |
Ack, that doesn't seem to be supported on functions/methods :( |
Looking at this again, the |
|
However, for the |
|
@SimiCode, I realize that several months have passed since you created this PR. Would you be interested and able to follow this up? |
…etimemodule Signed-off-by: Tal Einat <532281+taleinat@users.noreply.github.com>
MaxwellDupre
left a comment
There was a problem hiding this comment.
Ran 960 tests in 10.773s
OK (skipped=28)
Looks ok.
| Format using strftime(). | ||
|
|
||
| Example: "%d/%m/%Y, %H:%M:%S" | ||
| """Convert to a string in the given format via time.strftime(). |
There was a problem hiding this comment.
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)
| {"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" |
|
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 |
|
Thanks for your contribution, however, this has been superseded by #138559. |
Uh oh!
There was an error while loading. Please reload this page.