This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

Author moi90
Recipients andrei.avk, iritkatriel, jbw, moi90, rbcollins, serhiy.storchaka
Date 2021-11-10.16:38:29
SpamBayes Score -1.0
Marked as misclassified Yes
Message-id <1636562309.98.0.966118809664.issue43656@roundup.psfhosted.org>
In-reply-to
Content
Thanks Joe!

> 1. The changes are sufficient to let the user make things work the way it is requested that they work and anyone who does not start using the new format_locals parameter will get the old behavior.

That was my goal :)

> 2. A side comment: I just noticed that the docstring for FrameSummary.__init__ does not document the filename, lineno, and name parameters.  Perhaps this could be fixed at the same time?

I agree and already updated the pull request.

> 3. The new parameter format_locals is slightly confusingly named, because it does not format a single string from all the locals but instead gives a dictionary of strings with one string for each local.

I think format_locals is OK here (local*s*: plural), but I'm open to better suggestions.

> 4. I personally think it would be better to include something like the example value for format_locals as an extra attribute of the traceback module so everybody doesn't have to write the same function.  That said, the documented example is sufficient.

I sort of agree, but I also don't know what such a general-purpose function would entail. Suggestions?

> 5. It is not clear to me why this stringify-ing of the locals can't be done in StackSummary._extract_from_extended_frame_gen.  It passes the dictionary of locals and also the function to transform it to FrameSummary.  It would make more sense to transform it first.  I suppose there might be some user somewhere who is directly calling FrameSummary so the interface needs to stay.

I agree! In principle, FrameSummary has been little more than a container without (much) logic of its own.
Memory efficiency requires that that FrameSummary does not hold references to the original locals, thats why repr() was used.
I think as well that it would have been better to keep FrameSummary as a mere container and do the conversion of the locals elsewhere.
But at this point, this shouldn't be changed anymore.

> 6. I fantasize that the new format_locals parameter would have access to the frame object.  In order for this to happen, the frame object would have to be passed to FrameSummary instead of the 3 values (locals, name, filename) that are extracted from it.  I think the current interface of FrameSummary was designed to interoperate with very old versions of Python.  Oh well.

Do you have a use case for that? I have no clue what you would do with the frame object at this point.

> 7. If anyone wanted to ever refactor FrameSummary (e.g., to enable my fantasy in point #6 just above), it becomes harder after this.  This change has the effect of exposing details of the interface of FrameSummary to users of StackSummary.extract and TracebackException.  The four parameters that the new parameter format_locals takes are most of the parameters of FrameSummary.

Previously, I totally misread your request to "not just the local variable values, but also the name of the local variable and maybe also the stack frame it is in". This is why I included filename, lineno and name as parameters to format_locals which now seems totally useless to me and I'll remove them (if nobody objects).
History
Date User Action Args
2021-11-10 16:38:30moi90setrecipients: + moi90, rbcollins, serhiy.storchaka, iritkatriel, andrei.avk, jbw
2021-11-10 16:38:29moi90setmessageid: <1636562309.98.0.966118809664.issue43656@roundup.psfhosted.org>
2021-11-10 16:38:29moi90linkissue43656 messages
2021-11-10 16:38:29moi90create