Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-46769: Improve documentation for typing.TypeVar #31712

Merged
merged 3 commits into from
Mar 16, 2022

Conversation

AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Mar 6, 2022

This PR reworks the typing documentation to give more prominence to bound TypeVars as opposed to constrained TypeVars.

As outlined in bpo-46769, constrained TypeVars suffer from a number of issues which mean that bound TypeVars are often a more appropriate choice. However, the typing docs currently contain a number of examples of constrained TypeVars, but no examples of bound TypeVars, which are barely mentioned.

https://bugs.python.org/issue46769

Comment on lines 1177 to 1180
def print_capitalized(x: S) -> S:
"""Print x capitalized, and return x"""
print(x.capitalize())
return x
Copy link
Member Author

@AlexWaygood AlexWaygood Mar 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not wildly keen on this new example, but I was struggling to think of a better one that was (1) extremely simple and (2) showed the need for a bound TypeVar rather than a constrained TypeVar.

Comment on lines 1183 to 1185
def concatenate(x: A, y: A) -> A:
"""Add two strings together."""
return x + y
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I altered this example, as the longest example that is currently in the docs does not need a constrained TypeVar, and I'd like to use this section to illustrate the pros and cons of the two kinds of TypeVar.

@@ -245,7 +245,7 @@ subscription to denote expected types for container elements.
def notify_by_email(employees: Sequence[Employee],
overrides: Mapping[str, str]) -> None: ...

Generics can be parameterized by using a new factory available in typing
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TypeVar isn't really very new anymore

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I have a bunch of (mostly nitpicky) comments.

Doc/library/typing.rst Outdated Show resolved Hide resolved
Doc/library/typing.rst Show resolved Hide resolved
Doc/library/typing.rst Outdated Show resolved Hide resolved
Doc/library/typing.rst Outdated Show resolved Hide resolved
Doc/library/typing.rst Outdated Show resolved Hide resolved
can only ever be solved as being exactly one of the constraints given::

a = concatenate('one', 'two')
reveal_type(a) # revealed type is str
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using reveal_type() here is nice, but may make it tricky to backport this docs patch, because the 3.10 and 3.9 docs say nothing about reveal_type(). Then again, we've been using reveal_type() in PEPs for years too.

Copy link
Member Author

@AlexWaygood AlexWaygood Mar 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, do the typing docs not use reveal_type at all at the moment? I might have got the typing docs and PEP 484 confused in my head slightly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No (on 3.10), and neither does PEP 484. However, several later typing PEPs used (but did not define) reveal_type.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll reword this bit then, thanks for the catch!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's good to use reveal_type() in the long term though. Maybe leave it as is, and we can reword in the 3.10/3.9 backport PRs later?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, sure!

AlexWaygood and others added 2 commits March 6, 2022 23:24
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
@AlexWaygood AlexWaygood added needs backport to 3.9 only security fixes needs backport to 3.10 only security fixes labels Mar 7, 2022
Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! @gvanrossum planning to merge this docs change, though further feedback would be welcome too.

@JelleZijlstra JelleZijlstra merged commit 81b425d into python:main Mar 16, 2022
@AlexWaygood AlexWaygood deleted the typevar-docs branch March 16, 2022 15:53
@AlexWaygood
Copy link
Member Author

I need to make a few edits to the backports for this, so maybe I should just do them manually

@AlexWaygood AlexWaygood removed needs backport to 3.9 only security fixes needs backport to 3.10 only security fixes labels Mar 16, 2022
@JelleZijlstra
Copy link
Member

Sure, thanks!

@AlexWaygood AlexWaygood restored the typevar-docs branch March 16, 2022 16:41
@AlexWaygood AlexWaygood deleted the typevar-docs branch March 16, 2022 16:41
@AlexWaygood AlexWaygood restored the typevar-docs branch March 16, 2022 16:41
@AlexWaygood AlexWaygood deleted the typevar-docs branch March 16, 2022 16:41
AlexWaygood added a commit to AlexWaygood/cpython that referenced this pull request Mar 16, 2022
…H-31712)

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
(cherry picked from commit 81b425d)

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
JelleZijlstra pushed a commit that referenced this pull request Mar 23, 2022
… (GH-31941)

* [3.10] bpo-46769: Improve documentation for `typing.TypeVar` (GH-31712)

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
(cherry picked from commit 81b425d)

* Remove references to `reveal_type`, add new section on `self` types
JelleZijlstra pushed a commit to JelleZijlstra/cpython that referenced this pull request Mar 23, 2022
…ythonGH-31712) (pythonGH-31941)

* [3.10] bpo-46769: Improve documentation for `typing.TypeVar` (pythonGH-31712)

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
(cherry picked from commit 81b425d)

* Remove references to `reveal_type`, add new section on `self` types.
(cherry picked from commit d5ed8a8)

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
ambv pushed a commit that referenced this pull request Mar 23, 2022
GH-31941) (GH-32067)

* [3.9] [3.10] bpo-46769: Improve documentation for `typing.TypeVar` (GH-31712) (GH-31941)

* [3.10] bpo-46769: Improve documentation for `typing.TypeVar` (GH-31712)

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
(cherry picked from commit 81b425d)

* Remove references to `reveal_type`, add new section on `self` types.
(cherry picked from commit d5ed8a8)

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

* remove unused susp allowlist

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
hello-adam pushed a commit to hello-adam/cpython that referenced this pull request Jun 2, 2022
…-31712) (pythonGH-31941) (pythonGH-32067)

* [3.9] [3.10] bpo-46769: Improve documentation for `typing.TypeVar` (pythonGH-31712) (pythonGH-31941)

* [3.10] bpo-46769: Improve documentation for `typing.TypeVar` (pythonGH-31712)

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
(cherry picked from commit 81b425d)

* Remove references to `reveal_type`, add new section on `self` types.
(cherry picked from commit d5ed8a8)

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

* remove unused susp allowlist

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants