bpo-30211: bdb: add docstrings by csabella · Pull Request #1350 · python/cpython
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
| self.frame_returning = None | ||
|
|
||
| def canonic(self, filename): | ||
| """Get a filename into a canonical form. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"""Return canonical form of filename.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
-
canonicdoes 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'.
-
normcaseis notnormpathand latter, called byabspath, does not callnormcase.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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".
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| return self.trace_dispatch | ||
|
|
||
| def dispatch_line(self, frame): | ||
| """Trace function when a new line of code is executed. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're probably not going to like what I changed it to, but maybe it will help come up with something else. :-)
| @@ -323,6 +438,10 @@ def clear_all_file_breaks(self, filename): | |||
| del self.breaks[filename] | |||
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good.
|
|
||
|
|
||
| def set_trace(): | ||
| """Start debugging with a Bdb instance from the caller's frame.""" |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I presume you copied this, but I am not sure I understand it.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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().
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small point could change.
| 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. | ||
|
|
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for this blank line
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
|
||
| A canonical form is a case-normalized (on case insenstive filesystems) | ||
| absolute path, stripped of surrounding angle brackets. | ||
|
|
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
| See sys.settrace() for more information on the trace function. For | ||
| more information on code and frame objects, refer to The Standard | ||
| Type Hierarchy. | ||
|
|
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
| self._set_stopinfo(None, None) | ||
|
|
||
| def trace_dispatch(self, frame, event, arg): | ||
| """Trace function of debugged frames. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dispatch routine for debugged frames' event.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| 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. | ||
|
|
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
| 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. | ||
|
|
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
| 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. | ||
|
|
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
| 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. | ||
|
|
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
| funcname=None): | ||
| """Set a new breakpoint for the filename:lineno. | ||
|
|
||
| If lineno doesn't exist for the filename, return error. The |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return an error message.
for consistent.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
| def clear_break(self, filename, lineno): | ||
| """Delete breakpoints in filename:lineno. | ||
|
|
||
| If none were set, return an error message. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if none were set should be more clearly. But not sure how to change it.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to 'If no breakpoints were set"
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()
csabella
changed the title
bpo-19417: bdb: add docstrings
bpo-30211: bdb: add docstrings
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.
The class comment was added January 25, 1999 by GvR. :-)
|
|
||
| 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() |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a missing whitespace after the period.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Corrected.
| 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 |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can also remove the backticks in checkfuncname(), so we'll have all the docstrings formatted consistently.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I've also changed checkfuncname() to match the doc page.
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.
Thanks for reviewing and making those changes. I'm sorry there was so much left for you to work on.
| # definition of stopping and breakpoints. | ||
|
|
||
| def is_skipped_module(self, module_name): | ||
| "Return True if module_name matches any skip pattern." |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What this is difference between single quotes and triple quotes?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for explaining. Makes sense to use single quotes for simple lines that might need to wrap.
|
|
||
| This must be implemented by derived classes otherwise it raises | ||
| a NotImplementedError. | ||
| If not implemented in derived classe, raise NotImplementedError. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
classe -> class
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed with revision of sentence.
|
|
||
| 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. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing verb
|
|
||
| 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. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By 'original calling frame', I meant self.botframe. Initial loop breaks after appending botframe, then reverse makes botframe first.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Thanks. I think I need to figure out a call to get_stack to really understand it.
| 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. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
This may be followed by an additional PR for the same issue, but it is good enough for now.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters