-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
pdb: support kwargs with pdb.set_trace
#4419
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| pdb: support keyword arguments with ``pdb.set_trace`` | ||
|
|
||
| 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.*"])``. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -390,6 +390,28 @@ def test_1(): | |
| assert "hello17" in rest # out is captured | ||
| self.flush(child) | ||
|
|
||
| def test_pdb_set_trace_kwargs(self, testdir): | ||
| p1 = testdir.makepyfile( | ||
| """ | ||
| import pytest | ||
| def test_1(): | ||
| i = 0 | ||
| print("hello17") | ||
| pytest.set_trace(header="== my_header ==") | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @asottile |
||
| x = 3 | ||
| """ | ||
| ) | ||
| child = testdir.spawn_pytest(str(p1)) | ||
| child.expect("== my_header ==") | ||
| assert "PDB set_trace" not in child.before.decode() | ||
| child.expect("Pdb") | ||
| child.sendeof() | ||
| rest = child.read().decode("utf-8") | ||
| assert "1 failed" in rest | ||
| assert "def test_1" in rest | ||
| assert "hello17" in rest # out is captured | ||
| self.flush(child) | ||
|
|
||
| def test_pdb_set_trace_interception(self, testdir): | ||
| p1 = testdir.makepyfile( | ||
| """ | ||
|
|
@@ -634,6 +656,12 @@ def test_pdb_custom_cls_with_settrace(self, testdir, monkeypatch): | |
| testdir.makepyfile( | ||
| custom_pdb=""" | ||
| class CustomPdb(object): | ||
| def __init__(self, *args, **kwargs): | ||
| skip = kwargs.pop("skip") | ||
| assert skip == ["foo.*"] | ||
| print("__init__") | ||
| super(CustomPdb, self).__init__(*args, **kwargs) | ||
|
|
||
| def set_trace(*args, **kwargs): | ||
| print('custom set_trace>') | ||
| """ | ||
|
|
@@ -643,12 +671,13 @@ def set_trace(*args, **kwargs): | |
| import pytest | ||
|
|
||
| def test_foo(): | ||
| pytest.set_trace() | ||
| pytest.set_trace(skip=['foo.*']) | ||
| """ | ||
| ) | ||
| monkeypatch.setenv("PYTHONPATH", str(testdir.tmpdir)) | ||
| child = testdir.spawn_pytest("--pdbcls=custom_pdb:CustomPdb %s" % str(p1)) | ||
|
|
||
| child.expect("__init__") | ||
| child.expect("custom set_trace>") | ||
| self.flush(child) | ||
|
|
||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the changelog text is incorrect, it should talk about
pytest.set_traceThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pytestmonkeypatchespdb.set_trace:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #4416 (comment).
We're not changing the signature, but just pass through args and kwargs.
pdb.set_tracelooks like it might never handle args (*, header=Nonein py37), but just in case and/or if you want to hack something into this yourself.This PR handles
headeradded in py37, but in general (through the terminal writer), and forwards anything else toPdb.__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
**kwargsfirst, and then without them onTypeError, logging a warning.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any news on this folks? @asottile do you still strongly object to this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would help to get @RonnyPfannschmidt's opinion on this as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd put myself at non blocking opposed go ahead!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
inspectto have expected/proper TypeErrors?Also, this removes the
breakkwarg that could previously have been used to dopdb.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.