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

Improve docstrings for strptime and strftime methods in datetime module #31761

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

Conversation

MiguelGuthridge
Copy link

@MiguelGuthridge MiguelGuthridge commented Mar 8, 2022

Resolves #97517

I noticed that while using datetime's strptime and strftime methods that the docstrings were lacking in the sense that I had to refer to the documentation website in order to find the format codes I wanted to use. I've added a list of format codes to all functions in the datetime module where this problem was present, so that they will appear in the inline documentation of IDEs such as VS Code. I believe that this would hugely improve the experience of those writing code using the datetime library, especially if they are unable to remember all 27 format codes.

This is my first PR to a major project, so I'd really appreciate any feedback for how I can improve the changes.

@MiguelGuthridge
Copy link
Author

MiguelGuthridge commented Mar 13, 2022

Bumping this, would love some feedback on it

@MaxwellDupre
Copy link
Contributor

MaxwellDupre commented May 2, 2022

It would be better if they were in alphabetical order similar to the man page, to give a comparison, since checking all is not efficient.

@kumaraditya303 kumaraditya303 added docs Documentation in the Doc dir skip news labels May 2, 2022
@cpython-cla-bot
Copy link

cpython-cla-bot bot commented Sep 24, 2022

All commit authors signed the Contributor License Agreement.
CLA signed

Now they're in ascending order of duration
@MiguelGuthridge
Copy link
Author

MiguelGuthridge commented Sep 24, 2022

@MaxwellDupre apologies for the delay.

I've made changes similar to the requested ones now. I think it's more logical to order them by duration ascending rather than alphabetically, however, as most people will be looking for the escape sequence from the value rather than the other way round.

As such, it's now roughly seconds, minutes, hours, days, months, years.

Also added a basic example for the ctime method too, since its documentation wasn't very helpful either.

Copy link
Contributor

@MaxwellDupre MaxwellDupre left a comment

Better. No need to apologise, everyone works on this when they have time.

weekday = self.toordinal() % 7 or 7
return "%s %s %2d 00:00:00 %04d" % (
_DAYNAMES[weekday],
_MONTHNAMES[self._month],
self._day, self._year)

def strftime(self, fmt):
"Format using strftime()."
"""Format date or datetime given the format string `fmt`
Copy link
Member

@ericvsmith ericvsmith Sep 24, 2022

Choose a reason for hiding this comment

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

I think these changes are overkill for docstrings, and they impose a maintenance burden to keep them up to date. How about just containing a reference to the actual documentation?

Copy link
Author

@MiguelGuthridge MiguelGuthridge Sep 25, 2022

Choose a reason for hiding this comment

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

Perhaps a good compromise could be including the most common ones, and then having a link to the remaining ones?

Copy link
Author

@MiguelGuthridge MiguelGuthridge Sep 28, 2022

Choose a reason for hiding this comment

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

@ericvsmith bump?

Lib/datetime.py Outdated Show resolved Hide resolved
@MiguelGuthridge MiguelGuthridge changed the title Improve documentation for strptime and strftime methods in datetime module Improve docstrings for strptime and strftime methods in datetime module Sep 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting core review docs Documentation in the Doc dir skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants