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! -Barry
signature.asc
Description: Message signed with OpenPGP
_______________________________________________ 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
