Skip to content

/ cpython

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
Open

bpo-39278: add docstrings to functions in pdb module #17924

wants to merge 1 commit into from
+39 −0

Conversation

@carlbordum
Copy link
Contributor

carlbordum commented Jan 9, 2020 •
edited by bedevere-bot

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
@aeros aeros added the skip news label Jan 9, 2020
@aeros
aeros suggested changes Jan 9, 2020
View changes
Copy link
Member

aeros left a comment •
edited

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
Lib/pdb.py
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.

Sign in to view
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.

Sign in to view
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?

This comment has been minimized.

Sign in to view
Copy link
@aeros

aeros Jan 12, 2020

Member

@carlbordum

Thanks for the review!

No problem, thanks for making the recommended changes.

I am not sure "function" should be italic as the name is not in the signature. What do you think?

Personally, I'm a bit undecided on this one since it's in the function signature for the documentation: https://docs.python.org/3/library/pdb.html?highlight=runcall#pdb.runcall, I'm not certain why there's a discrepancy here between the docs and the source code.

Perhaps someone experienced with pdb can provide clarification. In the meantime, it's probably fine to leave it out.
Lib/pdb.py Outdated Show resolved Hide resolved
Lib/pdb.py Outdated Show resolved Hide resolved
Lib/pdb.py Show resolved Hide resolved
@carlbordum
bpo-39278: add docstrings to functions in pdb module
Loading status checks…
f392df8 
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
@aeros

This comment has been minimized.

Sign in to view
Copy link
Member

aeros commented Jan 12, 2020

@vstinner Based on the build logs at https://github.com/python/cpython/pull/17924/checks?check_run_id=382949957 and https://travis-ci.org/python/cpython/jobs/635143388?utm_medium=notification&utm_source=github_status, the failures look to be unrelated to the PR.

I suspect the "Install Dependencies" failure would likely be resolved if we re-triggered the CI checks, but the doc build failure in "library/nntplib.rst" might be a bit more involved:

Warning, treated as error:
**********************************************************************
File "library/nntplib.rst", line ?, in default
Failed example:
    s = NNTP('news.gmane.io')
Exception raised:
    Traceback (most recent call last):
      File "/home/travis/build/python/cpython/Lib/doctest.py", line 1329, in __run
        exec(compile(example.source, filename, "single",
      File "<doctest default[0]>", line 1, in <module>
        s = NNTP('news.gmane.io')
      File "/home/travis/build/python/cpython/Lib/nntplib.py", line 1050, in __init__
        _NNTPBase.__init__(self, file, host,
      File "/home/travis/build/python/cpython/Lib/nntplib.py", line 331, in __init__
        self.welcome = self._getresp()
      File "/home/travis/build/python/cpython/Lib/nntplib.py", line 456, in _getresp
        raise NNTPTemporaryError(resp)
    nntplib.NNTPTemporaryError: 400 load at 16.57, try later

Can we retrigger the tests here or would you suggest opening a bug report (if there's not one already)?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aeros aeros

Assignees
No one assigned
Labels
CLA signed awaiting core review skip news
Projects
None yet
Milestone
No milestone
4 participants
@carlbordum @aeros @the-knights-who-say-ni @bedevere-bot
You can’t perform that action at this time.