bpo-32121: Add reverse argument to tracemalloc.Traceback.format #4534
Conversation
reversing the order of the frames in the output
Lib/tracemalloc.py
Outdated
| lines = [] | ||
| if limit is not None and limit < 0: | ||
| return lines | ||
| for frame in self[:limit]: | ||
| frame_slice = self[:limit] |
There was a problem hiding this comment.
Hum, I'm not sure that it's useful to truncate before reversing the frames. I would suggest to truncate after reversing.
There was a problem hiding this comment.
The rationale was that the function is designed to format the frames of the limit last calls. If truncating after reversing, it will format the first limit calls, which I think would be less intuitive and less useful.
There was a problem hiding this comment.
LGTM, but I proposed to change the default parameter to reverse=True: see discussion at https://bugs.python.org/issue32121
Lib/tracemalloc.py
Outdated
| lines = [] | ||
| if limit is not None and limit < 0: | ||
| return lines | ||
| for frame in self[:limit]: | ||
| frame_slice = self[:limit] |
Reversed default sorting of tracemalloc.Traceback. Renamed argument. Adjusted docs. Allowed negative limit, truncating from the other side.
Lib/tracemalloc.py
Outdated
| """ | ||
| __slots__ = ("_frames",) | ||
|
|
||
| def __init__(self, frames): | ||
| Sequence.__init__(self) | ||
| # frames is a tuple of frame tuples: see Frame constructor for the | ||
| # format of a frame tuple | ||
| self._frames = frames | ||
| self._frames = tuple(reversed(frames)) |
There was a problem hiding this comment.
Please extend the comment to explain that the C module gives frames from the most recent to the oldest, but that ther reverse order is expected in the Python API.
| @@ -663,13 +663,14 @@ Traceback | |||
| The :attr:`Trace.traceback` attribute is an instance of :class:`Traceback` | |||
| instance. | |||
There was a problem hiding this comment.
Reversing the order of frames is a major backward incompatible change for this API, please mention it:
.. versionchanged:: 3.7
Frames are now sorted from the oldest to the most recent, instead of most recent to oldest.
Moreover, since it's a backward incompatible change, it must be documented in the "Porting" section of Doc/whatsnew/3.7.rst:
https://docs.python.org/dev/whatsnew/3.7.html#changes-in-the-python-api
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
I have made the requested changes; please review again. |
|
Thanks for making the requested changes! @vstinner: please review the changes made to this pull request. |
Lib/test/test_warnings/__init__.py
Outdated
| @@ -941,10 +941,10 @@ def func(): | |||
| {fname}:5: ResourceWarning: unclosed file <...> | |||
| f = None | |||
| Object allocated at (most recent call first): | |||
There was a problem hiding this comment.
Hum, the message stil says "most recent call first". It seems like you have to patch Lib/warnings.py too.
| sorting the frames from oldest to most recent. ``Traceback.format()`` | ||
| now accepts negative *limit*, truncating the result to the ``abs(limit)`` | ||
| oldest frames. To get the old behaviour, one can use the new | ||
| *most_recent_first* argument to ``Traceback.format()``. |
There was a problem hiding this comment.
Would you mind to credit yourself? Add "Patch by Jesse Bakker."
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase And if you don't make the requested changes, you will be poked with soft cushions! |
|
I have made the requested changes; please review again. |
|
Thanks for making the requested changes! @vstinner: please review the changes made to this pull request. |
https://bugs.python.org/issue32121