<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Oct 2, 2017 at 11:02 AM, Barry Warsaw <span dir="ltr"><<a href="mailto:barry@python.org" target="_blank">barry@python.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Thanks for the review Guido!  The PEP and implementation have been updated to address your comments, but let me briefly respond here.<br>
<span class=""><br>
> On Oct 2, 2017, at 00:44, Guido van Rossum <<a href="mailto:guido@python.org">guido@python.org</a>> wrote:<br>
<br>
> - There's a comma instead of a period at the end of the 4th bullet in the Rationale: "Breaking the idiom up into two lines further complicates the use of the debugger,” .<br>
<br>
</span>Thanks, fixed.<br>
<span class=""><br>
> Also I don't understand how this complicates use<br>
<br>
</span>I’ve addressed that with some additional wording in the PEP.  Basically, it’s my contention that splitting it up on two lines introduces more opportunity for mistakes.<br>
<span class=""><br>
> TBH the biggest argument (to me) is that I simply don't know *how* I would enter some IDE's debugger programmatically. I think it should also be pointed out that even if an IDE has a way to specify conditional breakpoints, the UI may be such that it's easier to just add the check to the code -- and then the breakpoint() option is much more attractive than having to look up how it's done in your particular IDE (especially since this is not all that common).<br>
<br>
</span>This is a really excellent point!  I’ve reworked that section of the PEP to make this clear.<br>
<span class=""><br>
> - There's no rationale for the *args, **kwds part of the breakpoint() signature. (I vaguely recall someone on the mailing list asking for it but it seemed far-fetched at best.)<br>
<br>
</span>I’ve added some rationale.  The idea comes from optional `header` argument to IPython’s programmatic debugger API.  I liked that enough to add it to pdb.set_trace() for 3.7.  IPython accepts other optional arguments, so I think we do want to allow those to be passed through the call chain.  I expect any debugger’s advertised entry point to make these optional, so `breakpoint()` will always just work.<br>
<span class=""><br>
> - The explanation of the relationship between sys.breakpoint() and sys.__breakpointhook__ was unclear to me<br>
<br>
</span>I think you understand it correctly, and I’ve hopefully clarified that in the PEP now, so you wouldn’t have to read the __displayhook__ (or __excepthook__) docs to understand how it works.<br>
<span class=""><br>
> - Some pseudo-code would be nice.<br>
<br>
</span>Great idea; added that to the PEP (pretty close to what you have, but with the warnings handling, etc.)<br>
<span class=""><br>
> I think something like `os.environ['PYTHONBREAKPOINT'<wbr>] = 'foo.bar.baz'; breakpoint()` should result in foo.bar.baz() being imported and called, right?<br>
<br>
</span>Correct.  Clarified in the PEP now.<br>
<span class=""><br>
> - I'm not quite sure what sort of fast-tracking for PYTHONBREAKPOINT=0 you had in mind beyond putting it first in the code above.<br>
<br>
</span>That’s pretty close to it.  Clarified.<br>
<span class=""><br>
> - Did you get confirmation from other debuggers? E.g. does it work for IDLE, Wing IDE, PyCharm, and VS 2015?<br>
<br>
</span>From some of them, yes.  Terry confirmed for IDLE, and I posted a statement in favor of the PEP from the PyCharm folks.  I’m pretty sure Steve confirmed that this would be useful for VS, and I haven’t heard from the Wing folks.<br>
<span class=""><br>
> - I'm not sure what the point would be of making a call to breakpoint() a special opcode (it seems a lot of work for something of this nature). ISTM that if some IDE modifies bytecode it can do whatever it well please without a PEP.<br>
<br>
</span>I’m strongly against including anything related to a new bytecode to PEP 553; they’re just IMHO orthogonal issues, and I’ve removed this as an open issue for 553.<br>
<br>
I understand why some debugger developers want it though.  There was a talk at Pycon 2017 about what PyCharm does.  They have to rewrite the bytecode to insert a call to a “trampoline function” which in many ways is the equivalent of breakpoint() and sys.breakpointhook().  I.e. it’s a small function that sets up and calls a more complicated function to do the actual debugging.  IIRC, they open up space for 4 bytecodes, with all the fixups that implies.  The idea was that there could be a single bytecode that essentially calls builtin breakpoint().  Steve indicated that this might also be useful for VS.<br>
<br>
There’s a fair bit that would have to be fleshed out to make this idea real, but as I say, I think it shouldn’t have anything to do with PEP 553, except that it could probably build on the APIs we’re adding here.<br>
<span class=""><br>
> - I don't see the point of calling `<a href="http://pdb.pm" rel="noreferrer" target="_blank">pdb.pm</a>()` at breakpoint time. But it could be done using the PEP with `import pdb; sys.breakpointhook = <a href="http://pdb.pm" rel="noreferrer" target="_blank">pdb.pm</a>` right? So this hardly deserves an open issue.<br>
<br>
</span>Correct, and I’ve removed this open issue.<br>
<span class=""><br>
> - I haven't read the actual implementation in the PR. A PEP should not depend on the actual proposed implementation for disambiguation of its specification (hence my proposal to add pseudo-code to the PEP).<br>
><br>
> That's what I have!<br>
<br>
</span>Cool, that’s very helpful, thanks!<br></blockquote></div></div><div class="gmail_extra"><br></div><div class="gmail_extra">I've seen your updates and it is now acceptable, except for *one* nit: in builtins.breakpoint() the pseudo code raises RuntimeError if sys.breakpointhook is missing or None. OTOH sys.breakpointhook() just issues a RuntimeWarning when something's wrong with the hook. Maybe builtins.breakpoint() should also just warn if it can't find the hook? Setting `sys.breakpointhook = None` might be the simplest way to programmatically disable breakpoints. Why not allow it?</div><div class="gmail_extra"><br></div><div class="gmail_extra">To Terry: Barry has given another excellent argument for passing through *args, **kwds so I remove my objection to it, regardless of what I think of your argument about the IDLE debugger and root windows. (It's been too long since I used Tkinter, so I don't trust my intuition there much anyways.)<br clear="all"></div><div class="gmail_extra"><br>-- <br><div class="gmail_signature" data-smartmail="gmail_signature">--Guido van Rossum (<a href="http://python.org/~guido" target="_blank">python.org/~guido</a>)</div>
</div></div>