bpo-44569: Decouple frame formatting in traceback.py #27038
Conversation
|
@terryjreedy Is this what you had in mind? @isidentical / @pablogsal Does this need a NEWS entry? |
If |
I don't see anything worth to NEWS entry, though it depends whether you intend to make this method public or private. I'd say we should make it private for now |
|
Except for the added '_', this is one version of what I had in mind. However, it was my intention that the possibility of overriding just the frame formatting, the point of this issue, be public. Currently and continuing in the future, could instead can override .format itself. I imagine that this has been done. But that is a lot more to copy-paste, which adds the possibility of missing improvements elsewhere in the function. For instance, the improvements in recursion truncation. The need to format each frame is stable. Moving the code from one public function to another does not eliminate the possibility of changing the details of what is produced, like we are already doing. I though about adding 'frame_formatter=_whatever' to the .format signature, but rejected it because users do not call .format directly, and a new argument would have to be propagated back to the module functions. EDIT: I verified that calling the module function that IDLE uses does indeed end up calling the frame formatting within .format, so this is the code I need to modify. |
|
Gotcha, let me switch it back to public and write up some documentation. |
|
@terryjreedy |
724d179 to
3174eb3
Compare
|
The address sanitizer failure was a test_mulitprocessing timeout. All works on rerun. When I updated my local main and made a branch with this pr, there was no merge conflict. I don't know how to make that claim disappear. Patch looks good to me. |
|
Aah, let me rebase to get rid of the github merge conflict. Thank you for the review, Terry. |
3174eb3 to
bb00d37
Compare
|
Rebased. |
|
I believe pushing my clean branch would have done the same, except I lacked permission ;-). |
bb00d37 to
f136c1e
Compare
|
Rebased for #27126 |
| @@ -449,6 +449,48 @@ def from_list(klass, a_list): | |||
| result.append(FrameSummary(filename, lineno, name, line=line)) | |||
| return result | |||
|
|
|||
| def format_frame(self, frame): | |||
There was a problem hiding this comment.
I just realized that this is actually a FrameSummary object rather than a frame.
Since we haven't released it yet, I think we should rename the function and its arg to format_frame_summary(frame_summary). What do you think?
There was a problem hiding this comment.
I think is a good idea. What do you think @ammaraskar @isidentical ?
There was a problem hiding this comment.
That sounds good to me, format frame summary seems more descriptive.
There was a problem hiding this comment.
Cool. I see the confusion between frame and frame_summary in local variable names predates this PR. I'll try to fix that as well.
There was a problem hiding this comment.
I've created bpo 45075 for this.
This allows customization of how individual frames are printed by
traceback.pywithout having to re-implement the recursive/repeated lines handling oftraceback.StackSummary.format.https://bugs.python.org/issue44569