pdb: support kwargs with pdb.set_trace
#4419
Conversation
This handles `header` similar to Python 3.7 does it, and forwards any
other keyword arguments to the Pdb constructor.
This allows for `__import__("pdb").set_trace(skip=["foo.*"])`.
Fixes pytest-dev#4416.
blueyed
force-pushed
the
set_trace-kwargs
branch
from
32dc389 to
92a2884
Compare
Codecov Report
@@ Coverage Diff @@
## features #4419 +/- ##
===========================================
+ Coverage 95.88% 95.9% +0.01%
===========================================
Files 111 111
Lines 24997 25545 +548
Branches 2442 2537 +95
===========================================
+ Hits 23969 24498 +529
- Misses 726 742 +16
- Partials 302 305 +3
Continue to review full report at Codecov.
|
| It handles ``header`` similar to Python 3.7 does it, and forwards any | ||
| other keyword arguments to the ``Pdb`` constructor. | ||
|
|
||
| This allows for ``__import__("pdb").set_trace(skip=["foo.*"])``. |
There was a problem hiding this comment.
this changes the signature of pdb.set_trace() I don't think we should do this (we also have to manually adjust our code every time cpython adds a parameter to this function which seems non-ideal)
There was a problem hiding this comment.
the changelog text is incorrect, it should talk about pytest.set_trace
There was a problem hiding this comment.
pytest monkeypatches pdb.set_trace:
import pdb
import pytest
def test():
assert pdb.set_trace.__module__ == '_pytest.debugging'
$ pytest t.py -q
. [100%]
1 passed in 0.00 secondsThere was a problem hiding this comment.
See #4416 (comment).
We're not changing the signature, but just pass through args and kwargs.
pdb.set_trace looks like it might never handle args (*, header=None in py37), but just in case and/or if you want to hack something into this yourself.
This PR handles header added in py37, but in general (through the terminal writer), and forwards anything else to Pdb.__init__.
pdb++ makes use of this, and any custom pdbcls could do so then.
I do not see a compatibility issue really, but it makes it just a bit more forward-compatible - also a new kwarg (like "header") might cause an error then when Pdb.__init__ does not handle it.
For this, we could try it with **kwargs first, and then without them on TypeError, logging a warning.
There was a problem hiding this comment.
@asottile right now, there is not a good way to provide those arguments to Pdb directly. If you could read into the original request #4416, I think it could help to give a better understanding on the context of the change.
Right now we are constrained to build the Pdb class in the predefined way, with no arguments. However with this patch we would be able to initalize Pdb, or any other implementation (I am keen on TerminalPdb from ipython, or pycharm's pdb class) with the custom arguments to support debugging asyncio.
I would highly encourage to try debugging asyncio code with pytest.set_trace() with no skip support, and you will find that all the control changes from the ioloop and back are almost impossible to follow due to all the await syntax.
This will ease a lot my development of fully asyncio libraries/applications, as many times you find bugs through pytest, and you need to debug them in that specific fixtured environment.
There was a problem hiding this comment.
Any news on this folks? @asottile do you still strongly object to this?
There was a problem hiding this comment.
Would help to get @RonnyPfannschmidt's opinion on this as well.
There was a problem hiding this comment.
Heh, I can see @RonnyPfannschmidt literally on some fence already.. ;)
I'm also a bit on @asottile's side here in general, but do not really see a good way around this. Maybe we could use inspect to have expected/proper TypeErrors?
Also, this removes the break kwarg that could previously have been used to do pdb.set_trace(break=False) - although it appears to come only from internal use without refactoring it properly (54d3cd5), i.e. an API break as it stands now.
There was a problem hiding this comment.
I think the thing that convinces me that this is alright is that an interactive call, so the user knows he is running its own debugger tracing function. If this was an API call I would also be against it, as you don't know what debugger the person using your API is.
But let's see @RonnyPfannschmidt's opinion, if he is also against this as @asottile is, then I'm afraid we will need to close this PR.
| def test_1(): | ||
| i = 0 | ||
| print("hello17") | ||
| pytest.set_trace(header="== my_header ==") |
There was a problem hiding this comment.
@asottile
This currently fails, i.e. we do not support py37's signature.
| It handles ``header`` similar to Python 3.7 does it, and forwards any | ||
| other keyword arguments to the ``Pdb`` constructor. | ||
|
|
||
| This allows for ``__import__("pdb").set_trace(skip=["foo.*"])``. |
There was a problem hiding this comment.
See #4416 (comment).
We're not changing the signature, but just pass through args and kwargs.
pdb.set_trace looks like it might never handle args (*, header=None in py37), but just in case and/or if you want to hack something into this yourself.
This PR handles header added in py37, but in general (through the terminal writer), and forwards anything else to Pdb.__init__.
pdb++ makes use of this, and any custom pdbcls could do so then.
I do not see a compatibility issue really, but it makes it just a bit more forward-compatible - also a new kwarg (like "header") might cause an error then when Pdb.__init__ does not handle it.
For this, we could try it with **kwargs first, and then without them on TypeError, logging a warning.
5 participants
This handles
headersimilar to Python 3.7 does it, and forwards anyother keyword arguments to the Pdb constructor.
This allows for
__import__("pdb").set_trace(skip=["foo.*"]).Fixes #4416.