Loosen parameters of traceback.format_list and traceback.StackSummary.from_list #2436

nathanielmanistaatgoogle · 2018-09-07T22:38:48Z

traceback.FrameSummary's line parameter is a string that is the text
of a line of code, not an int that is the line number of a line of
code.

Also relax type of traceback.format_list's extracted_list parameter
in 3.5-and-later from List[FrameSummary] to Iterable[FrameSummary].
Never ask for a List when asking for a Sequence will do, and never ask
for a Sequence when asking for a Collection will do, and never ask for
a Collection when asking for an Iterable will do (and so on in other
cases, but this case stops at Iterable).

Also add a note about deliberately not supporting for type-checking
purposes the 3.4-and-earlier List[Tuple[str, int, str, Optional[str]]]
semantics for traceback.format_list's extracted_list parameter.
traceback currently supports the old type of parameter, but if
authors are typechecking their 3.5-and-later code, it's reasonable to
expect that they'll want to modernize their code to use FrameSummary
objects rather than four-tuples.

srittau

Thank you for your contribution! I left a note below.

srittau · 2018-09-11T00:57:29Z

stdlib/2and3/traceback.pyi

+    # enough to type-check their 3.5-or-later code, they're probably diligent
+    # enough to pass FrameSummarys rather than make use of the
+    # only-for-backwards-compatibility four-tuple parameter type semantics.
+    def format_list(extracted_list: Iterable[FrameSummary]) -> List[str]: ...


Supplying a list (or iterable) of tuples is not discouraged by the docs and still explicitly supported. Therefore, I'd indeed prefer the type to be Iterable[Union[_PT, FrameSummary]]. Also, could you make the same change to StackSummary.from_list() for consistency?

I see where you're coming from, but... it just feels like maybe someone's reaching for a chance to deprecate older semantics and I want to help that process along.

Also ad hoc polymorphism is no one's friend (despite its long history in Python...).

I've filed a documentation-clarification bug; let's see what kind of response it gets after a few days?

nathanielmanistaatgoogle

Thanks!

nathanielmanistaatgoogle · 2018-09-12T20:07:07Z

stdlib/2and3/traceback.pyi

+    # enough to type-check their 3.5-or-later code, they're probably diligent
+    # enough to pass FrameSummarys rather than make use of the
+    # only-for-backwards-compatibility four-tuple parameter type semantics.
+    def format_list(extracted_list: Iterable[FrameSummary]) -> List[str]: ...


I see where you're coming from, but... it just feels like maybe someone's reaching for a chance to deprecate older semantics and I want to help that process along.

Also ad hoc polymorphism is no one's friend (despite its long history in Python...).

I've filed a documentation-clarification bug; let's see what kind of response it gets after a few days?

nathanielmanistaatgoogle · 2018-09-18T04:59:47Z

I think this is now ready for integration - there are changes that I would have liked to have made here, but they're instead described in TODOs while the conversation continues in the bug.

nathanielmanistaatgoogle · 2018-09-19T12:23:51Z

@srittau: what do you think of the state of this change? I think this is mergeable now; have you taken a look?

rchen152 · 2018-09-20T17:01:32Z

This PR looks good to me.
@srittau Are there any other changes you want to see?

srittau

Thank you for the changes. One more thing I noticed.

srittau · 2018-09-20T18:03:03Z

stdlib/2and3/traceback.pyi

+        # drop support for _PT.
        @classmethod
-        def from_list(cls, a_list: List[_PT]) -> StackSummary: ...
+        def from_list(cls, a_list: List[Union[_PT, FrameSummary]]) -> StackSummary: ...


This will probably not work as expected due to the invariance of List. (Although when I just tested it, mypy for some reason interpreted List[Union[_PT, FrameSummary]] as List[Any].) This should be Union[List[_PT], List[FrameSummary]].

Why not just Iterable or Sequence? Even if the documentation says "list", we routinely accept a wider type in stubs; documentation is often written in a more informal style.

@JelleZijlstra: I agree!

... but however routine it is, it didn't strike me as a good idea for typeshed to describe as acceptable a wider type than what a standard library module's maintainers officially describe as supported.

I think both are acceptable. Personally, I would go with a List as that is what the stdlib maintainers seem to prefer, but accepting an Iterable is fine, too. The latter has the advantage of being covariant.

nathanielmanistaatgoogle · 2018-09-21T23:49:03Z

This is an important and productive conversation that we are having here but in the meantime I have extracted the non-controversial parts of this change into pull request 2470.

Will respond to the outstanding comments here sometime in the next few days I promise! :-)

Never ask for a List when asking for a MutableSequence will do. Never ask for a MutableSequence when asking for a Sequence will do. Never ask for a Sequence when asking for a Collection will do. Never ask for a Collection when asking for an Iterable will do. Also drop support for Tuple[str, int, str, Optional[str]] elements in the iterables passed to traceback.format_list and traceback.StackSummary.from_list - if authors are diligent enough to be type-checking their code, they are diligent enough to be making use of their dependencies according to their preferred semantics rather than their backwards-compatibility carve-outs.

nathanielmanistaatgoogle · 2018-09-24T15:03:10Z

Retitled to reflect current content, and for now this change is aspirational based on how the discussion in Python bug 34648 proceeds. But I think it's a worthwhile aspiration! :-P

Dismissing my review because the purpose of this PR has changed since I approved it.

JelleZijlstra · 2018-10-26T03:58:26Z

stdlib/2and3/traceback.pyi

-    def format_list(extracted_list: List[FrameSummary]) -> List[str]: ...
+    # NOTE(https://bugs.python.org/issue34648): Authors who are diligent
+    # enough to type-check their code are authors who are diligent enough to
+    # upgrade their Tuple[str, int, str, Optional[str]]s to FrameSummarys.


I don't think this kind of rhetoric belongs in a comment. I would be willing to make this accept an Iterable instead of a List, but there's no need for the comments.

srittau · 2018-12-02T12:55:39Z

I am closing this PR for now, based on the discussion in bpo-34648. I think changing List to Iterable in the parameter types still has merit, but disallowing tuples, which are explicitly documented to work is not the task of the type system. I also agree on @JelleZijlstra's point about the comment rhetoric.

srittau requested changes Sep 11, 2018

View reviewed changes

nathanielmanistaatgoogle commented Sep 12, 2018

View reviewed changes

nathanielmanistaatgoogle force-pushed the traceback branch from f365bdf to b6ba8a2 Compare September 12, 2018 20:25

srittau mentioned this pull request Sep 13, 2018

Investigate CI failures #2456

Closed

nathanielmanistaatgoogle force-pushed the traceback branch from b6ba8a2 to 6161139 Compare September 18, 2018 04:58

nathanielmanistaatgoogle force-pushed the traceback branch 2 times, most recently from 0d5cbe5 to 59985ae Compare September 18, 2018 05:09

rchen152 previously approved these changes Sep 20, 2018

View reviewed changes

srittau requested changes Sep 20, 2018

View reviewed changes

nathanielmanistaatgoogle mentioned this pull request Sep 21, 2018

Fix traceback.FrameSummary's "line" parameter #2470

Merged

nathanielmanistaatgoogle force-pushed the traceback branch from 59985ae to 55caf8c Compare September 22, 2018 00:10

nathanielmanistaatgoogle force-pushed the traceback branch from 55caf8c to a5bbe2f Compare September 24, 2018 15:00

nathanielmanistaatgoogle changed the title ~~Fix traceback.FrameSummary's "line" parameter~~ Loosen parameters of traceback.format_list and traceback.StackSummary.from_list Sep 24, 2018

JelleZijlstra reviewed Oct 26, 2018

View reviewed changes

srittau closed this Dec 2, 2018

Uh oh!

Loosen parameters of traceback.format_list and traceback.StackSummary.from_list #2436

Loosen parameters of traceback.format_list and traceback.StackSummary.from_list #2436

Uh oh!

Conversation

nathanielmanistaatgoogle commented Sep 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

nathanielmanistaatgoogle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nathanielmanistaatgoogle commented Sep 18, 2018

Uh oh!

nathanielmanistaatgoogle commented Sep 19, 2018

Uh oh!

rchen152 commented Sep 20, 2018

Uh oh!

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

nathanielmanistaatgoogle commented Sep 21, 2018

Uh oh!

nathanielmanistaatgoogle commented Sep 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srittau commented Dec 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nathanielmanistaatgoogle commented Sep 7, 2018 •

edited

Loading