bpo-32121: Add reverse argument to tracemalloc.Traceback.format by Jesse-Bakker · Pull Request #4534 · python/cpython

Jesse-Bakker · 2017-11-24T00:06:17Z

https://bugs.python.org/issue32121

reversing the order of the frames in the output

vstinner · 2017-11-24T00:22:10Z

        if limit is not None and limit < 0:
            return lines
-        for frame in self[:limit]:
+        frame_slice = self[:limit]


Hum, I'm not sure that it's useful to truncate before reversing the frames. I would suggest to truncate after reversing.

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.

My bad, you are right :-)

vstinner

LGTM, but I proposed to change the default parameter to reverse=True: see discussion at https://bugs.python.org/issue32121

vstinner · 2017-11-24T10:53:28Z

        if limit is not None and limit < 0:
            return lines
-        for frame in self[:limit]:
+        frame_slice = self[:limit]


My bad, you are right :-)

Reversed default sorting of tracemalloc.Traceback. Renamed argument. Adjusted docs. Allowed negative limit, truncating from the other side.

vstinner · 2017-11-29T14:25:42Z

        # 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))


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.

vstinner · 2017-11-29T14:26:59Z

@@ -663,13 +663,14 @@ Traceback
   The :attr:`Trace.traceback` attribute is an instance of :class:`Traceback`
   instance.


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

bedevere-bot · 2017-11-29T14:28:08Z

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. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Doc/whatsnew)

Jesse-Bakker · 2017-11-29T16:21:28Z

I have made the requested changes; please review again.

bedevere-bot · 2017-11-29T16:21:30Z

Thanks for making the requested changes!

@vstinner: please review the changes made to this pull request.

vstinner

It seems like the warnings code displaying a traceback must be updated for the "most recent frame first" line.

vstinner · 2017-11-29T17:21:22Z

@@ -941,10 +941,10 @@ def func():
            {fname}:5: ResourceWarning: unclosed file <...>
              f = None
            Object allocated at (most recent call first):


Hum, the message stil says "most recent call first". It seems like you have to patch Lib/warnings.py too.

vstinner · 2017-11-29T17:22:00Z

+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()``.


Would you mind to credit yourself? Add "Patch by Jesse Bakker."

bedevere-bot · 2017-11-29T17:22:48Z

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. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be poked with soft cushions!

Jesse-Bakker · 2017-11-29T18:46:52Z

I have made the requested changes; please review again.

bedevere-bot · 2017-11-29T18:46:54Z

Thanks for making the requested changes!

@vstinner: please review the changes made to this pull request.

vstinner

LGTM, thanks!

Added reverse argument to tracemalloc.Traceback.format to allow

4a245c2

reversing the order of the frames in the output

the-knights-who-say-ni added the CLA signed label Nov 24, 2017

bedevere-bot added the awaiting review label Nov 24, 2017

vstinner reviewed Nov 24, 2017

View reviewed changes

Jesse Bakker added 3 commits November 29, 2017 15:05

Changed according to comments on the issue.

391850b

Reversed default sorting of tracemalloc.Traceback. Renamed argument. Adjusted docs. Allowed negative limit, truncating from the other side.

Fix tests for new Traceback implementation

939f069

Fix whitespace

8916805

vstinner requested changes Nov 29, 2017

View reviewed changes

bedevere-bot removed the awaiting review label Nov 29, 2017

bedevere-bot added the awaiting changes label Nov 29, 2017

Jesse Bakker added 4 commits November 29, 2017 16:31

Fix test_warnings.PyWarningsDisplayTests.test_tracemalloc

507b885

Added docs for backwards incompatible changes (.. versionchanged:: and

d16d5bc

Doc/whatsnew)

Add comment about reversing frames

357b528

Fix formatting in docs

501c6b4

bedevere-bot added awaiting change review and removed awaiting changes labels Nov 29, 2017

vstinner requested changes Nov 29, 2017

View reviewed changes

bedevere-bot removed the awaiting change review label Nov 29, 2017

bedevere-bot added the awaiting changes label Nov 29, 2017

Jesse Bakker added 2 commits November 29, 2017 18:57

Merge branch 'master' into add-tracemalloc-traceback-format-reverse

19d8103

Fix ResourceWarning message and credited self

14bf75b

bedevere-bot added awaiting change review and removed awaiting changes labels Nov 29, 2017

vstinner approved these changes Nov 29, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Nov 29, 2017

vstinner merged commit 706e10b into python:master Nov 29, 2017

bedevere-bot removed the awaiting merge label Nov 29, 2017

		@@ -663,13 +663,14 @@ Traceback
		The :attr:`Trace.traceback` attribute is an instance of :class:`Traceback`
		instance.

Uh oh!

Conversation

Jesse-Bakker commented Nov 24, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

vstinner 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!

bedevere-bot commented Nov 29, 2017

Uh oh!

Jesse-Bakker commented Nov 29, 2017

Uh oh!

bedevere-bot commented Nov 29, 2017

Uh oh!

vstinner 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!

bedevere-bot commented Nov 29, 2017

Uh oh!

Jesse-Bakker commented Nov 29, 2017

Uh oh!

bedevere-bot commented Nov 29, 2017

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Jesse-Bakker commented Nov 24, 2017 •

edited by bedevere-bot

Loading