Add option to include stack trace in debug() output by kalekundert · Pull Request #143 · samuelcolvin/python-devtools

kalekundert · 2023-10-09T18:48:28Z

As discussed in #105, this PR adds some options to have debug() output stack traces.

While I was working on this, I realized that there are two reasonable way to format stack traces. The first is to match the existing style that debug() uses:

<file>:<line> <function>

The second is to match the style that python itself uses:

File "<file>", line <line>, in <function>
    <open(file).readlines()[line-1]>

Both styles have the same information, but the python style is more verbose and includes the actual text of the line in question. I decided to use the devtools style, because it "fits" better and I didn't think the extra verbosity would be that helpful for the kinds of things I envision debugging with this feature, but I'm not confident in this decision. I might change my mind after living with it for a while. If anyone else has an opinion on which style to use, I'd be happy to hear it.

Another aspect of this PR that I should draw attention to is the new unit tests. I think that all the stack trace tests need to happen in subprocesses, because otherwise the stack traces would include ≈20 pytest frames (which would be likely to change between different versions of pytest). To avoid duplicating the boilerplate required to run a test in a subprocess, I decided to write these tests using two of my own libraries: parametrize_from_file and pytest_tmp_files. The specific code I wrote here is very similar to this example from the documentation. I think that I wrote these tests in the most understandable, maintainable way, but I'd understand if you were skeptical about adding two relatively unknown dependencies to your project. If so, let me know and I'd be happy to rewrite the tests using vanilla pytest. (Also, I had to regenerate the requirements/testing.txt file, and I'm not sure I did that in the right way.)

codecov · 2023-10-09T18:49:36Z

Codecov Report

Merging #143 (3edf2e9) into main (ec406ff) will decrease coverage by 0.42%.
The diff coverage is 92.00%.

❗ Current head 3edf2e9 differs from pull request most recent head 128a178. Consider uploading reports for the commit 128a178 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #143      +/-   ##
==========================================
- Coverage   96.29%   95.88%   -0.42%     
==========================================
  Files           8        8              
  Lines         729      753      +24     
  Branches      111      115       +4     
==========================================
+ Hits          702      722      +20     
- Misses         21       23       +2     
- Partials        6        8       +2

Files	Coverage Δ
devtools/debug.py	`97.38% <92.00%> (-2.62%)`	⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ec406ff...128a178. Read the comment docs.

samuelcolvin

thanks so much for working on this, would you mind adding a screenshot to the PR to show how it looks?

samuelcolvin · 2023-10-09T19:15:46Z

devtools/debug.py

+
+        return DebugFrame(function, str(path), lineno)
+
+    def __init__(self, function: str, path: str, lineno: int):


__init__ should come first.

samuelcolvin · 2023-10-09T19:17:13Z

devtools/debug.py

                prefix += f' ({self.warning})'
-        return f'{prefix}\n    ' + '\n    '.join(a.str(highlight) for a in self.arguments)
+
+        return prefix + '\n    ' + '\n    '.join(a.str(highlight) for a in self.arguments)


I don't think this needs to change.

Thought it would be more efficient to avoid forcing python to evaluate the f-string, but not important.

samuelcolvin · 2023-10-09T19:17:55Z

devtools/debug.py

            call_frame: 'FrameType' = sys._getframe(frame_depth)
        except ValueError:
-            # "If [ValueError] is deeper than the call stack, ValueError is raised"
+            # "If [the given frame depth] is deeper than the call stack,


undo this change.

Happy to undo this, but this comment made no sense to me. I had to go find that quote in the actual python docs to figure out what it meant.

samuelcolvin · 2023-10-09T19:19:31Z

devtools/debug.py

            yield self.output_class.arg_class(value, name=name, variable=kw_arg_names.get(name))


+def _make_call_context(call_frame: 'Optional[FrameType]', trace: bool) -> 'List[DebugFrame]':


might make most sense for this to be part of DebugFrame

There's not really much difference between a function and a static method, but all else being equal, I prefer functions. I think they're easier to understand because you know they only have access to the public APIs of any classes they use. But I can make it a method if you care strongly about it.

samuelcolvin · 2023-10-09T19:19:55Z

devtools/debug.py

 StrType = str


+class DebugFrame:


probably best to put this in a new module, otherwise it should go at the bottom of debug.py.

I think this class is pretty tightly-coupled to all the other classes in debug.py, so I just moved it to the bottom.

samuelcolvin · 2023-10-09T19:22:54Z

requirements/testing.in

+pytest-tmp-files
+parametrize-from-file


I don't think we need these extra dependencies, it should be a few lines of code to do this manually, either with strings in the python code, or reading files from a new directory in tests.

you can probably use python files with a multiline string at the end of the for expected stdout.

I rewrote the tests using vanilla python.

kalekundert · 2023-10-09T20:20:54Z

Here's a screenshot:

kalekundert · 2025-04-02T14:35:42Z

I just realized that this PR never got merged. Is there anything else I need to do to get it ready?

add option to include stack trace in debug() output

24efa58

try to satisfy linter

3edf2e9

samuelcolvin reviewed Oct 9, 2023

View reviewed changes

address review comments

18e981e

try to satisfy the linter

128a178


		return DebugFrame(function, str(path), lineno)

		def __init__(self, function: str, path: str, lineno: int):

		yield self.output_class.arg_class(value, name=name, variable=kw_arg_names.get(name))


		def _make_call_context(call_frame: 'Optional[FrameType]', trace: bool) -> 'List[DebugFrame]':

		pytest-tmp-files
		parametrize-from-file

Uh oh!

Conversation

kalekundert commented Oct 9, 2023

Uh oh!

codecov bot commented Oct 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

samuelcolvin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kalekundert commented Oct 9, 2023

Uh oh!

kalekundert commented Apr 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Oct 9, 2023 •

edited

Loading