|
|
|
Created:
15 years, 10 months ago by gregory.p.smith Modified:
15 years, 10 months ago CC:
python tracker Base URL:
http://svn.python.org/view/*checkout*/python/trunk/ Visibility:
Public. |
Patch Set 1 #
Total comments: 11
Patch Set 2 : sig.patch.v2 #
Total comments: 8
Patch Set 3 : sig.patch.3 #Total messages: 4
|
Benjamin
Mostly style nits. http://codereview.appspot.com/216067/diff/1/2 File Lib/pdb.py (right): http://codereview.appspot.com/216067/diff/1/2#newcode62 Lib/pdb.py:62: def sigHandler( self, sigNum, f): There ...
|
15 years, 10 months ago (2010年02月21日 15:19:48 UTC) #1 |
Mostly style nits. http://codereview.appspot.com/216067/diff/1/2 File Lib/pdb.py (right): http://codereview.appspot.com/216067/diff/1/2#newcode62 Lib/pdb.py:62: def sigHandler( self, sigNum, f): There should be space between the definition of the class and this method. Also, no space between self and the left parenthesis. I think this method should probably have an underscored name, too. http://codereview.appspot.com/216067/diff/1/2#newcode63 Lib/pdb.py:63: print >>self.stdout,"\nProgram interrupted. (Use 'cont' to resume)." Space between >> and self.stdout. http://codereview.appspot.com/216067/diff/1/2#newcode64 Lib/pdb.py:64: self.set_step() #print "got signal" Remove that comment. http://codereview.appspot.com/216067/diff/1/2#newcode1303 Lib/pdb.py:1303: signal.signal( signal.SIGINT, pdb.sigHandler) No space between the left parenthesis and first argument. http://codereview.appspot.com/216067/diff/1/2#newcode1303 Lib/pdb.py:1303: signal.signal( signal.SIGINT, pdb.sigHandler) Is this signal behavior not desirable when pdb.set_trace() or pdb.post_mortem() is called?
I have also attached a new patch/some additional comments to the http://bugs.python.org/issue7245 (I didn't find a way to upload the patch to appspot) http://codereview.appspot.com/216067/diff/1/2 File Lib/pdb.py (right): http://codereview.appspot.com/216067/diff/1/2#newcode62 Lib/pdb.py:62: def sigHandler( self, sigNum, f): > probably have an underscored name, too. Why do you think so? I'd think that sigHandler could be useful to anyone who wants to extend Pdb class. (I definitely don't feel strongly about it ;-). And, speaking of naming, should sigHandler be renamed into "sighandler" (all low case)? as that seems a bit more consistent with the rest of the module and library. http://codereview.appspot.com/216067/diff/1/2#newcode62 Lib/pdb.py:62: def sigHandler( self, sigNum, f): > There should be space between the definition of the class and this method. Also, no space between self and the left parenthesis Done http://codereview.appspot.com/216067/diff/1/2#newcode63 Lib/pdb.py:63: print >>self.stdout,"\nProgram interrupted. (Use 'cont' to resume)." > Space between >> and self.stdout. Why do you think it'd be better? The rest of pdb.py does not use space after ">>". Either way is fine with me but I'd prefer to avoid inconsistency http://codereview.appspot.com/216067/diff/1/2#newcode64 Lib/pdb.py:64: self.set_step() #print "got signal" On 2010年02月21日 15:19:48, Benjamin wrote: > Remove that comment. Done. http://codereview.appspot.com/216067/diff/1/2#newcode1303 Lib/pdb.py:1303: signal.signal( signal.SIGINT, pdb.sigHandler) I did it this way to minimize changes in behavior. My guess would be that this behavior is useful in set_trace() but not in post_mortem() (which I believe, mostly used for data inspection just before the program exit). I may be very wrong here (and I don't use either of these). http://codereview.appspot.com/216067/diff/1/2#newcode1303 Lib/pdb.py:1303: signal.signal( signal.SIGINT, pdb.sigHandler) On 2010年02月21日 15:19:48, Benjamin wrote: > No space between the left parenthesis and first argument. Done.
Also, can you take a look at how the pdb unittests work and see if you can come up with a way to unittest the KeyboardInterrupt behavior? Particular for the 4 scenarios you outlined in your prior comments in the bug (those all make sense to me). http://codereview.appspot.com/216067/diff/2001/2002 File Lib/pdb.py (right): http://codereview.appspot.com/216067/diff/2001/2002#newcode63 Lib/pdb.py:63: def sigint_handler(self, signum, frame): Please move this below the __init__ definition. It makes classes odd to read when __init__ isn't the first method defined when people are looking for the constructor to see how to use it. http://codereview.appspot.com/216067/diff/2001/2002#newcode64 Lib/pdb.py:64: if self.allow_kbdint: Initialize self.allow_kdbint in __init__ so that a SIGINT coming in before _cmdloop has run doesn't cause an AttributeError. http://codereview.appspot.com/216067/diff/2001/2002#newcode215 Lib/pdb.py:215: # keyboard interrupts allow for an easy way to interrupt "to cancel the current command" http://codereview.appspot.com/216067/diff/2001/2002#newcode356 Lib/pdb.py:356: #it appears that that when pdb is reading input from a pipe Space after the # please. Also, could you add a comment in here describing what the effect of this code is? It looks like you catch KeyboardInterrupt here and remove the particular interrupted command from the list of commands to run at the current breakpoint but I may be misreading things as I haven't spent much time in pdb.py.
new version of the patch is uploaded to bugs.python.org http://codereview.appspot.com/216067/diff/2001/2002 File Lib/pdb.py (right): http://codereview.appspot.com/216067/diff/2001/2002#newcode63 Lib/pdb.py:63: def sigint_handler(self, signum, frame): On 2010年02月28日 20:12:00, gregory.p.smith wrote: > Please move this below the __init__ definition. It makes classes odd to read > when __init__ isn't the first method defined when people are looking for the > constructor to see how to use it. > Done. http://codereview.appspot.com/216067/diff/2001/2002#newcode64 Lib/pdb.py:64: if self.allow_kbdint: On 2010年02月28日 20:12:00, gregory.p.smith wrote: > Initialize self.allow_kdbint in __init__ so that a SIGINT coming in before > _cmdloop has run doesn't cause an AttributeError. Done. http://codereview.appspot.com/216067/diff/2001/2002#newcode215 Lib/pdb.py:215: # keyboard interrupts allow for an easy way to interrupt On 2010年02月28日 20:12:00, gregory.p.smith wrote: > "to cancel the current command" I changed the wording a bit, should be ok now. http://codereview.appspot.com/216067/diff/2001/2002#newcode356 Lib/pdb.py:356: #it appears that that when pdb is reading input from a pipe On 2010年02月28日 20:12:00, gregory.p.smith wrote: > Space after the # please. > > Also, could you add a comment in here describing what the effect of this code > is? > > It looks like you catch KeyboardInterrupt here and remove the particular > interrupted command from the list of commands to run at the current breakpoint > but I may be misreading things as I haven't spent much time in pdb.py. Done. I've also added a comment to explain what's going on.