Hope you don't mind me CC'ing python-dev. On Sun, Oct 1, 2017 at 9:38 AM, Barry Warsaw <barry@python.org> wrote:
You seem to be in PEP review mode :)
What do you think about 553? Still want to wait, or do you think it’s missing anything? So far, all the feedback has been positive, and I think we can basically just close the open issues and both the PEP and implementation are ready to go.
Happy to do more work on it if you feel it’s necessary. -B
I'm basically in agreement. Some quick notes: - 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," . Also I don't understand how this complicates use (even though I also always type it as one line). And the lint warning is actually useful when you accidentally leave this in code you send to CI (assuming that runs a linter, as it should). 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). - 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.) - The explanation of the relationship between sys.breakpoint() and sys.__breakpointhook__ was unclear to me -- I had to go to the docs for __displayhook__ ( https://docs.python.org/3/library/sys.html#sys.__displayhook__) to understand that sys.__breakpointhook__ is simply initialized to the same function as sys.breakpointhook, and the idea is that if you screw up you can restore sys.breakpointhook from the other rather than having to restart your process. Is that in fact it? The text "``sys.__breakpointhook__`` then stashes the default value of ``sys.breakpointhook()`` to make it easy to reset" seems to imply some action that would happen... when? how? - Some pseudo-code would be nice. It seems that it's like this: # in builtins def breakpoint(*args, **kwds): import sys return sys.breakpointhook(*args, **kwds) # in sys def breakpointhook(*args, **kwds): import os hook = os.getenv('PYTHONBREAKPOINT') if hook == '0': return None if not hook: import pdb return pdb.set_trace(*args, **kwds) if '.' not in hook: import builtins mod = builtins funcname = hook else: modname, funcname = hook.rsplit('.', 1) __import__(modname) import sys mod = sys.modules[modname] func = getattr(mod, funcname) return func(*args, **kwds) __breakpointhook__ = breakpointhook Except that the error handling should be a bit better. (In particular the PEP specifies a try/except around most of the code in sys.breakpointhook() that issues a RuntimeWarning and returns None.) - Not sure what the PEP's language around evaluation of PYTHONBREAKPOINT means for the above pseudo code. I *think* the PEP author's opinion is that the above pseudo-code is fine. Since programs can mutate their own environment, I think something like `os.environ['PYTHONBREAKPOINT'] = 'foo.bar.baz'; breakpoint()` should result in foo.bar.baz() being imported and called, right? - 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. - Did you get confirmation from other debuggers? E.g. does it work for IDLE, Wing IDE, PyCharm, and VS 2015? - 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 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. - 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! -- --Guido van Rossum (python.org/~guido <http://python.org/%7Eguido>)