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-47097: Add documentation for TypeVarTuple #32103

Merged
merged 14 commits into from
Apr 4, 2022
Merged

Conversation

mrahtz
Copy link
Contributor

@mrahtz mrahtz commented Mar 24, 2022

Here's the first part of the documentation for PEP 646: documentation of TypeVarTuple in https://docs.python.org/3.11/library/typing.html.

I've kept this information self-contained in the section on TypeVarTuple rather than e.g. also writing about TypeVarTuple in the section on Generic because, let's face it, user-defined generics are kinda tricky, and if a user is reading about Generic for the first time, we don't want to overload them.

There are many other things from the PEP we could write about here, but I'm assuming that best practice is to keep the docs accessible by only including the key bits of info, and linking to the PEP itself for more details?

I'm also unsure about whether we should add something about Unpack here. On one hand, it's only relevant for older versions of Python, so I lean towards thinking we should only write about it when we get to backporting? On the other hand, it's still going to be a public member of typing.py, so maybe it would be better to say at least something?

Pedagogical feedback also appreciated :) I wouldn't be surprised if there's an easier way to explain this.

@JelleZijlstra @pradeep90

https://bugs.python.org/issue47097

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, this is a good explanation! Here is some initial feedback.

We do also need to document Unpack, since it's part of the public interface of typing.py now.

Agree that the explanations in the typing docs can be a bit more concise. We're aiming to provide more detailed documentation at typing.readthedocs.io, but we're not quite there yet.

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
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Sorry for the picky review. Glad this is happening!

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
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
Doc/library/typing.rst Show resolved Hide resolved
Doc/library/typing.rst Show resolved Hide resolved
@AlexWaygood
Copy link
Member

(I'll wait until @gvanrossum's and @JelleZijlstra's comments have been addressed before reviewing :)

@mrahtz
Copy link
Contributor Author

mrahtz commented Mar 26, 2022

Thanks both for the thorough feedback!

[Jelle]

We do also need to document Unpack, since it's part of the public interface of typing.py now.

Gotcha. I'll write a short section on this now.

[Guido]

Sorry for the picky review. Glad this is happening!

Nah, picky reviews are good for documentation :)

Copy link
Member

@AlexWaygood AlexWaygood 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 was excited to see the PEP 646 grammar changes get merged the other day! 😀

In general, the examples here all feel a little too abstract for my liking. This is really complex stuff, so I think it would be good to link this to real-life use cases as much as possible (while still keeping the examples concise).

Doc/library/typing.rst Outdated Show resolved Hide resolved
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 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
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

LGTM if you apply Alex's shortening around lines 1264-1266.

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
Doc/library/typing.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@pradeep90 pradeep90 left a comment

Choose a reason for hiding this comment

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

High-level: The docs here seems to be focused more on the technical aspects of what is allowed and what is not allowed (e.g., multiple unpackings not allowed, *Ts can be combined with T). Could we focus on one or two use cases (such as remove_first_element for tuple and add_tensors) and defer to the PEP for the technical details?

For example, ParamSpec mainly just shows how to use ParamSpec for add_logging (and Concatenate for with_lock). That basically looks like the Motivation section of PEP 612 (along with a couple of technical notes about P.args and P.kwargs).

add_tensors could be:

class Array(Generic[*Shape]):
    def __add__(self, other: Array[*Shape]) -> Array[*Shape]: ...
    
def foo(array1: Array[L[10], L[20]], array2: Array[L[10], L[20]], bad_array: Array[L[99]]) -> None:
  result = array1 + array2  # OK because they have the same shape.
  reveal_type(result)  # Array[L[10], L[20]]

  array1 + bad_array  # Error: Expected ...

Thus, variadic tuples allow us to enforce that two arrays have the same shape, which is useful in ...

(Note that broadcasting is not yet supported; we plan to add it in a follow-up PEP.)

We may or may not also want to mention *args - I don't feel strongly about it.

This might make this doc more approachable for the average reader.

Doc/library/typing.rst Outdated Show resolved Hide resolved
Doc/library/typing.rst Outdated Show resolved Hide resolved
@gvanrossum
Copy link
Member

Could we focus on one or two use cases (such as remove_first_element for tuple and add_tensors) and defer to the PEP for the technical details?

It's fine to show examples and such first, but the current best practice is not to rely on PEPs for technical details/specifications -- PEPs describe a proposal for changes, whereas docs describe the status quo. Over time the language changes and PEPs become out of date, so the docs should be self-contained. Another way to look at it: PEPs are for language designers, docs are for language users.

This is a marked difference with languages for which there is formal, complete standard (like C++ or Java) -- PEPs are more like diffs to the standard (other languages have a process like this too, e.g. Java's JEPs -- I don't know what it's called in C++).

@pradeep90
Copy link
Contributor

the current best practice is not to rely on PEPs for technical details/specifications -- PEPs describe a proposal for changes, whereas docs describe the status quo.

Ah, thanks for the information. Then it makes sense to add examples up front and keep the rest as is.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Nice, I like the new example a lot more! I think it makes the example a lot more graspable if we fill out the function body, though, and translate the types into actual values

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
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
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@mrahtz
Copy link
Contributor Author

mrahtz commented Apr 2, 2022

Great suggestions, Alex! Committed.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Just one tiny nit about the placement of explanatory comments in the code example -- in all other examples in typing.rst, the explanatory comments are either inline or immediately above the code they're explaining. Can we keep this example consistent? :)

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
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Doc/library/typing.rst Outdated Show resolved Hide resolved
Doc/library/typing.rst Outdated Show resolved Hide resolved
mrahtz and others added 2 commits April 2, 2022 11:41
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Great stuff — thanks for your patience!

@mrahtz
Copy link
Contributor Author

mrahtz commented Apr 2, 2022

Thanks for your thorough feedback! It's particularly important that the docs are good for this complicated typing stuff.

@mrahtz
Copy link
Contributor Author

mrahtz commented Apr 4, 2022

@JelleZijlstra I think we're converged now. You happy to merge this?

@JelleZijlstra JelleZijlstra self-requested a review April 4, 2022 18:09
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.

Looks great, just two nits

Doc/library/typing.rst Outdated Show resolved Hide resolved
Doc/library/typing.rst Outdated Show resolved Hide resolved
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
@mrahtz
Copy link
Contributor Author

mrahtz commented Apr 4, 2022

Good catches :)

@JelleZijlstra JelleZijlstra self-assigned this Apr 4, 2022
@JelleZijlstra JelleZijlstra merged commit 38ae5b8 into python:main Apr 4, 2022
@bedevere-bot
Copy link

@JelleZijlstra: Please replace # with GH- in the commit message next time. Thanks!

@mrahtz
Copy link
Contributor Author

mrahtz commented Apr 4, 2022

Thanks Jelle!

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.

9 participants