bpo-30211: bdb: add docstrings #1350

csabella · 2017-04-28T20:28:21Z

No description provided.

mention-bot · 2017-04-28T20:28:24Z

@csabella, thanks for your PR! By analyzing the history of the files in this pull request, we identified @tim-one, @birkenfeld and @avassalotti to be potential reviewers.

terryjreedy

This is the start I wanted. Thank you. Good catch on the typos. This review is incomplete in that some comments mark a place to change without yet saying exactly how.

Maybe add docstring for _set_stopinfo, moving comment into docstring. (Can't seem to add this inline.)

terryjreedy · 2017-04-29T01:38:17Z

Lib/bdb.py

@@ -26,6 +34,12 @@ def __init__(self, skip=None):
        self.frame_returning = None

    def canonic(self, filename):
+        """Get a filename into a canonical form.


"""Return canonical form of filename.

Thanks! Three questions on this function:

It seems like this helper function would be valuable in os.path, but maybe not if it hasn't been needed before. Are there times when helper functions like this are moved?

This function also caches this results. Would there be any point in using a decorator or does that add unnecessary overhead?

os.path.abspath says that it returns a normalized absolutized version of the path. So why call both here?

This function is not really a method, as it does not use 'self'. But is was written before 'staticmethod' and the original author(s) must have not wanted to expose it at module level. We are here documenting the API and will next test the current implementation. Once tested, we could possibly revise.

canonic does 2 things different from os function(s).
1A. Leave angle bracketed names alone. This following part of the doc, copied in your patch, "stripped of surrounding angle brackets" does not match the code. Either way, I don't know what bracketed ''s are about or when, if ever, they occur. I also don't know if the existing test is the fastest possible.
1B. Cache the mapping of non-bracketed names to absolute name. This must be for speed as the same abbreviated name is used repeatedly.

It does not cache bracketed name. If it did, I would say 'test alternatives for speed'.

normcase is not normpath and latter, called by abspath, does not call normcase.

Thanks for that explanation. Looking through the patch history, canonic was added later. I'll try to find it again and maybe the commit comment will help. I have no idea how the old commits were ported to git, but it is just one more cool thing -- to have all the history available. :-)

Edit
Found it - November 28, 2001 -
canonic(): don't use abspath() for filenames looking like <...>; this
fixes the problem reported in SF bug #477023 (Jonathan Mark): "pdb:
unexpected path confuses Emacs".

https://bugs.python.org/issue477023 works. The bracketed path names are from interactive mode.

terryjreedy · 2017-04-29T02:18:54Z

Lib/bdb.py

@@ -62,12 +104,28 @@ def trace_dispatch(self, frame, event, arg):
        return self.trace_dispatch

    def dispatch_line(self, frame):
+        """Trace function when a new line of code is executed.


Hmm. Just saying "Return trace function." does not really help much. It is really an implementation detail in that communicating a tracer change could have been differently. setting self.tracer, for instance, could have been the linking mechanism. Describing the 'side-effect', which is actually the main effect is awkward. "Return trace function after calling user_line if debugger stops here." is fairly accurate and descriptive. But I am open to other ideas for the wording. Ditto for other 3 event handlers.

You're probably not going to like what I changed it to, but maybe it will help come up with something else. :-)

terryjreedy · 2017-04-29T02:31:46Z

Lib/bdb.py

@@ -323,6 +438,10 @@ def clear_all_file_breaks(self, filename):
        del self.breaks[filename]


When there is an explicit non-None return in a function, then any None return should be explicit. (This is somewhere in PEP 8.) So add 'return None' on new line after this.

I hadn't changed any code, so I wasn't sure about this when I saw it. I've now added None to all the similar set_* functions that had another return and no return None. Question -- if there are no returns, should I add return None?

Another question: there is some code like this - "if self.quitting: raise BdbQuit". Should I put that on two lines?

Clarification: add 'return None' at the end of the function ;-). I missed 'set_break' and any others because of the folding; thank you for checking. The relevant PEP8 section begins "Be consistent in return statements." It is standard to omit 'return None' at the end and leave it implicit when there are no other return statements.

For 'if cond: statement', PEP8 says both 'Rather not:' and 'While sometimes it's okay'. I think we should leave them alone.

Sounds good.

terryjreedy · 2017-04-29T02:34:56Z

Lib/bdb.py

@@ -472,6 +632,7 @@ def runcall(self, func, *args, **kwds):


 def set_trace():
+    """Start debugging with a Bdb instance from the caller's frame."""


I presume you copied this, but I am not sure I understand it.

Yes, it's from the docs. There are three functions defined outside of the classes - set_trace(), effective(), and checkfuncname(). effective() and checkfuncname() already had docstrings, so I didn't change them. set_trace() has one line - Bdb.set_trace().

louisom

Some small point could change.

louisom · 2017-04-29T05:06:46Z

Lib/bdb.py

+    that originate in a module that matches one of these patterns.
+    Whether a frame is considered to originate in a certain module
+    is determined by the __name__ in the frame globals.
+


No need for this blank line

Thanks! I wasn't sure on that one. PEP-8 doesn't have it, but existing strings in this module did. Probably a good time to remove them all.

louisom · 2017-04-29T05:07:48Z

Lib/bdb.py

+
+        A canonical form is a case-normalized (on case insenstive filesystems)
+        absolute path, stripped of surrounding angle brackets.
+


louisom · 2017-04-29T05:08:45Z

Lib/bdb.py

+        See sys.settrace() for more information on the trace function.  For
+        more information on code and frame objects, refer to The Standard
+        Type Hierarchy.
+


louisom · 2017-04-29T05:10:21Z

Lib/bdb.py

        import linecache
        linecache.checkcache()
        self.botframe = None
        self._set_stopinfo(None, None)

    def trace_dispatch(self, frame, event, arg):
+        """Trace function of debugged frames.


Dispatch routine for debugged frames' event.

Made a slight change to yours to make it a verb, but thank you for suggesting to put 'dispatch' first. I had struggled with this yesterday.

louisom · 2017-04-29T05:11:38Z

Lib/bdb.py

+        method.  Raise a BdbQuit exception if the Bdb.quitting flag is set.
+        Return a reference to the trace_dispatch() method for further
+        tracing in that scope.
+


louisom · 2017-04-29T05:12:01Z

Lib/bdb.py

+        method.  Raise a BdbQuit exception if the Bdb.quitting flag is set.
+        Return a reference to the trace_dispatch() method for further
+        tracing in that scope.
+


louisom · 2017-04-29T05:12:09Z

Lib/bdb.py

+        user_exception() method.  Raise a BdbQuit exception if the
+        Bdb.quitting flag is set. Return a reference to the trace_dispatch()
+        method for further tracing in that scope.
+


louisom · 2017-04-29T05:12:43Z

Lib/bdb.py

+        Check if there is a breakpoint in the filename and lineno
+        belonging to the frame or in the current function.  If the breakpoint
+        is a temporary one, delete it.
+


louisom · 2017-04-29T05:15:05Z

Lib/bdb.py

    # Set_break prints out the breakpoint line and file:lineno.
    # Call self.get_*break*() to see the breakpoints or better
    # for bp in Breakpoint.bpbynumber: if bp: bp.bpprint().

    def set_break(self, filename, lineno, temporary=False, cond=None,
                  funcname=None):
+        """Set a new breakpoint for the filename:lineno.
+
+        If lineno doesn't exist for the filename, return error.  The


return an error message.

for consistent.

louisom · 2017-04-29T05:17:45Z

Lib/bdb.py

@@ -293,6 +396,10 @@ def _prune_breaks(self, filename, lineno):
            del self.breaks[filename]

    def clear_break(self, filename, lineno):
+        """Delete breakpoints in filename:lineno.
+
+        If none were set, return an error message.


I think if none were set should be more clearly. But not sure how to change it.

Changed to 'If no breakpoints were set"

csabella · 2017-04-29T12:25:49Z

As far as documenting _set_stopinfo, I didn't know if the _ functions should get docstrings or not. Looked at some other modules and it didn't seem like they were, so I left it out. Changed that for this and for _prune_breaks. Is it standard to include docstrings for the functions defined with _?

Terry, I'm sure you've seen this, but this is marked as a 'gotcha' in the Breakpoint class. Not sure if I need to docstring it somehow:

# XXX Keeping state in the class is a mistake -- this means
# you cannot have more than one active Bdb instance.

next = 1        # Next bp to be assigned
bplist = {}     # indexed by (file, lineno) tuple
bpbynumber = [None] # Each entry is None or an instance of Bpt
            # index 0 is unused, except for marking an
            # effective break .... see effective()

terryjreedy · 2017-04-29T22:14:29Z

Is it standard to include docstrings for the functions defined with _?
Ask on core_mentership. It certainly is useful for writing a test.

XXX Keeping state in the class is a mistake -- this means

you cannot have more than one active Bdb instance.

Whoever wrote that considers it a bug, which we could try to fix in a follow-on issue.
We don't usually document bugs 'officially', as fixing them is better ;-). An open issue kind of serves as an unofficial notice.

csabella · 2017-04-29T23:14:09Z

The class comment was added January 25, 1999 by GvR. :-)

marco-buttu · 2017-05-08T16:08:39Z

Lib/bdb.py

+
+        If the debugger stops on this function return, invoke the
+        user_exception() method.  Raise a BdbQuit exception if the
+        Bdb.quitting flag is set. Return a reference to the trace_dispatch()


There is a missing whitespace after the period.

Thanks. Corrected.

marco-buttu · 2017-05-08T16:26:33Z

Lib/bdb.py

@@ -194,6 +277,12 @@ def user_exception(self, frame, exc_info):
        pass

    def _set_stopinfo(self, stopframe, returnframe, stoplineno=0):
+        """Set the attributes for stopping.
+
+        If `stoplineno` is greater than or equal to 0, then stop at line


In this doctstring you are wrapping the argument stoplineno with `, while in all the other doctstrings you are not. I think to be consistent you have to remove the `` around stoplineno.

Thanks. I've removed the backticks on the ones I added. There is still one set of backticks that I didn't add. Do you think I should remove those too?

I think we can also remove the backticks in checkfuncname(), so we'll have all the docstrings formatted consistently.

Done. I've also changed checkfuncname() to match the doc page.

terryjreedy · 2017-05-16T04:37:37Z

Refreshing the patch was required for 'import re' (in patchcheck and IDLE) to run in pr_1350 with a current build. I am now editing the docstrings themselves. Please wait before committing anything more.

Many reduce wordiness. Only a few intend to change meaning.

csabella · 2017-05-16T12:37:16Z

Thanks for reviewing and making those changes. I'm sorry there was so much left for you to work on.

csabella · 2017-05-16T12:58:44Z

Lib/bdb.py

@@ -125,12 +187,14 @@ def dispatch_exception(self, frame, arg):
    # definition of stopping and breakpoints.

    def is_skipped_module(self, module_name):
+        "Return True if module_name matches any skip pattern."


What this is difference between single quotes and triple quotes?

Triple quotes are only needed for multiple lines. For single lines, single quotes are commonly used, in spite of what PEP257 says. I only changed your additions where line length was an issue.

Thank you for explaining. Makes sense to use single quotes for simple lines that might need to wrap.

csabella · 2017-05-16T13:04:04Z

Lib/bdb.py


-        This must be implemented by derived classes otherwise it raises
-        a NotImplementedError.
+        If not implemented in derived classe, raise NotImplementedError.


classe -> class

Fixed with revision of sentence.

csabella · 2017-05-16T13:05:04Z

Lib/bdb.py

        """
        raise NotImplementedError("subclass of bdb must implement do_clear()")

    def break_anywhere(self, frame):
-        """Return True if there is a breakpoint in the filename for the frame.
+        """Return True if there any breakpoint for frame's filename.


missing verb

csabella · 2017-05-16T13:17:50Z

Lib/bdb.py


-        Get a list of records for a frame and all higher (calling) and
-        lower frames, and the size of the higher part.
+        List starts with original calling frame, if there is one.


Does the return value start with the original calling frame? With the stack.reverse(), I thought it first traversed in one direction from f (backwards - which would put f first), but then reversed the list so f was no longer first and then continued to append the traceback frames.

By 'original calling frame', I meant self.botframe. Initial loop breaks after appending botframe, then reverse makes botframe first.

Got it. Thanks. I think I need to figure out a call to get_stack to really understand it.

csabella · 2017-05-16T13:23:00Z

Lib/bdb.py

-        Get a list of records for a frame and all higher (calling) and
-        lower frames, and the size of the higher part.
+        List starts with original calling frame, if there is one.
+        Size may be number of frames above or below f.


For the size, the line 'if f is None: i = max()', but I don't see why f wouldn't be None here. It could come in as None or the 'while f is not None:' loop changes it until it is None. So, the size would either be 0 (if there isn't a stack) or the size of the stack - 1. It wouldn't be just the above part or below part. I realize I'm probably missing something, but I don't see it. Thanks.

f is either None or self.botframe (the break condition). The latter might or might not be None. The length calculation makes no sense to me. So I put something vague that should be accurate.

terryjreedy · 2017-05-16T22:29:44Z

This may be followed by an additional PR for the same issue, but it is good enough for now.

bpo-19417: bdb: add docstrings

3adafa2

the-knights-who-say-ni added the CLA signed label Apr 28, 2017

terryjreedy requested changes Apr 29, 2017

View reviewed changes

terryjreedy self-assigned this Apr 29, 2017

louisom suggested changes Apr 29, 2017

View reviewed changes

bpo-19417: bdb: add docstrings

c335e37

csabella changed the title ~~bpo-19417: bdb: add docstrings~~ bpo-30211: bdb: add docstrings Apr 29, 2017

brettcannon added the docs Documentation in the Doc dir label May 1, 2017

marco-buttu reviewed May 8, 2017

View reviewed changes

csabella and others added 3 commits May 8, 2017 13:12

bpo-30211: bdb: add docstrings

f898ce1

bpo-30211: bdb: add docstrings

7f7a1d1

Merge remote-tracking branch 'origin/master' into pr_1350

7805f71

Revise proposed docstrings. Some changes are reformatting to 72 chars.

5d8a9e7

Many reduce wordiness. Only a few intend to change meaning.

terryjreedy approved these changes May 16, 2017

View reviewed changes

csabella commented May 16, 2017

View reviewed changes

terryjreedy added 3 commits May 16, 2017 15:21

Fix typos

d43ad02

Fix typos

8d88214

Merge remote-tracking branch 'origin' into pr_1350

7096c82

terryjreedy merged commit 0774e79 into python:master May 16, 2017

csabella deleted the bpo19417 branch May 20, 2017 00:28

csabella mentioned this pull request May 14, 2022

Bdb: add docstrings #74397

Closed

		@@ -323,6 +438,10 @@ def clear_all_file_breaks(self, filename):
		del self.breaks[filename]

		@@ -472,6 +632,7 @@ def runcall(self, func, args, *kwds):


		def set_trace():
		"""Start debugging with a Bdb instance from the caller's frame."""


		A canonical form is a case-normalized (on case insenstive filesystems)
		absolute path, stripped of surrounding angle brackets.

bpo-30211: bdb: add docstrings #1350

bpo-30211: bdb: add docstrings #1350

Conversation

csabella commented Apr 28, 2017

mention-bot commented Apr 28, 2017

terryjreedy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csabella Apr 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

louisom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csabella commented Apr 29, 2017 • edited Loading

terryjreedy commented Apr 29, 2017

XXX Keeping state in the class is a mistake -- this means

you cannot have more than one active Bdb instance.

csabella commented Apr 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csabella May 8, 2017 • edited Loading

Choose a reason for hiding this comment

terryjreedy commented May 16, 2017

csabella commented May 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terryjreedy commented May 16, 2017

csabella Apr 29, 2017 •

edited

Loading

csabella commented Apr 29, 2017 •

edited

Loading

csabella May 8, 2017 •

edited

Loading