On Mon, Oct 2, 2017 at 11:02 AM, Barry Warsaw <ba...@python.org> wrote:
> Thanks for the review Guido! The PEP and implementation have been updated > to address your comments, but let me briefly respond here. > > > On Oct 2, 2017, at 00:44, Guido van Rossum <gu...@python.org> wrote: > > > - 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,” . > > Thanks, fixed. > > > Also I don't understand how this complicates use > > 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. > > > 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). > > This is a really excellent point! I’ve reworked that section of the PEP > to make this clear. > > > - 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.) > > 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. > > > - The explanation of the relationship between sys.breakpoint() and > sys.__breakpointhook__ was unclear to me > > 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. > > > - Some pseudo-code would be nice. > > Great idea; added that to the PEP (pretty close to what you have, but with > the warnings handling, etc.) > > > I think something like `os.environ['PYTHONBREAKPOINT'] = 'foo.bar.baz'; > breakpoint()` should result in foo.bar.baz() being imported and called, > right? > > Correct. Clarified in the PEP now. > > > - 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. > > That’s pretty close to it. Clarified. > > > - Did you get confirmation from other debuggers? E.g. does it work for > IDLE, Wing IDE, PyCharm, and VS 2015? > > 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. > > > - 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. > > 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. > > 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. > > 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. > > > - I don't see the point of calling `pdb.pm()` at breakpoint time. But > it could be done using the PEP with `import pdb; sys.breakpointhook = > pdb.pm` right? So this hardly deserves an open issue. > > Correct, and I’ve removed this open issue. > > > - 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). > > > > That's what I have! > > Cool, that’s very helpful, thanks! > 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? 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.) -- --Guido van Rossum (python.org/~guido)
_______________________________________________ Python-Dev mailing list Python-Dev@python.org https://mail.python.org/mailman/listinfo/python-dev Unsubscribe: https://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
