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-39278: add docstrings to functions in pdb module #17924

Open
wants to merge 1 commit into
base: master
from

Conversation

@carlbordum
Copy link
Contributor

carlbordum commented Jan 9, 2020

All these functions were already documented, but not in docstrings which
means you could not call help() on the functions before this patch.

https://bugs.python.org/issue39278

Copy link
Member

aeros left a comment

Thanks for the PR @carlbordum. IMO, it's always helpful to keep the docstrings up to date with the documentation.

I've verified that the changes match the documentation at https://docs.python.org/3/library/pdb.html. The only thing that's missing is a few formatting conversions, particularly the words that should be italics. In docstrings, we typically surround parameter names with asterisks (only for the first occurrence in each docstring though). I'll include the missing ones in suggestions below.

The only other thing you could consider including would be a docstring for the Pdb class (https://docs.python.org/3/library/pdb.html#pdb.Pdb). But, the PR is still quite helpful in it's current state, so I think it could be merged with or without that addition once the asterisks are added.

Lib/pdb.py Outdated Show resolved Hide resolved
Lib/pdb.py Outdated Show resolved Hide resolved
Lib/pdb.py Outdated Show resolved Hide resolved
Lib/pdb.py Show resolved Hide resolved
return Pdb().runeval(expression, globals, locals)

def runctx(statement, globals, locals):
# B/W compatibility
run(statement, globals, locals)

def runcall(*args, **kwds):
"""Call the function (a function or method object, not a string)

This comment has been minimized.

Copy link
@aeros

aeros Jan 9, 2020

Member
Suggested change
"""Call the function (a function or method object, not a string)
"""Call the *function* (a function or method object, not a string)

This comment has been minimized.

Copy link
@carlbordum

carlbordum Jan 10, 2020

Author Contributor

Thanks for the review! I went ahead and changed what you asked for except for this one case. I am not sure "function" should be italic as the name is not in the signature. What do you think?

Lib/pdb.py Outdated Show resolved Hide resolved
Lib/pdb.py Outdated Show resolved Hide resolved
@@ -1614,6 +1646,12 @@ def set_trace(*, header=None):
# Post-Mortem interface

def post_mortem(t=None):

This comment has been minimized.

Copy link
@aeros

aeros Jan 9, 2020

Member

Here's another discrepancy between the docs and the function signature. This one is a bit less significant, but I find it odd that the signature uses t as the parameter name while the documentation uses traceback.

I'd be in favor of adjusting the documentation here, as it could lead to issues if a user tries to pass a keyword argument to traceback. For example: pdb.post_mortem(traceback=tb) would lead to a rather confusing TypeError.

Once again, not directly related to the PR but worth noting.

All these functions were already documented, but not in docstrings which
means you could not call help() on the functions before this patch.
@carlbordum carlbordum force-pushed the carlbordum:bpo-39278 branch from d801de2 to f392df8 Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.