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

Added draft chapter to typing spec for constructors. #1667

Merged
merged 11 commits into from Apr 11, 2024

Conversation

erictraut
Copy link
Collaborator

DRAFT VERSION FOR REVIEW — DO NOT MERGE

docs/spec/constructors.rst Outdated Show resolved Hide resolved
docs/spec/constructors.rst Outdated Show resolved Hide resolved
docs/spec/constructors.rst Outdated Show resolved Hide resolved
erictraut and others added 3 commits March 28, 2024 09:01
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
docs/spec/constructors.rst Show resolved Hide resolved
Comment on lines 159 to 160
After evaluating the ``__new__`` method, a type checker should evaluate the
``__init__`` method using the supplied arguments. If the class is
Copy link

Choose a reason for hiding this comment

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

Suggested change
After evaluating the ``__new__`` method, a type checker should evaluate the
``__init__`` method using the supplied arguments. If the class is
After evaluating the ``__new__`` method (and the return type is unspecified), a type checker should evaluate the
``__init__`` method using the supplied arguments. If the class is

Same as my other comment: as written in the __new__ section:

If the return type of the __new__ method is specified as Any (or the return type is unspecified and not inferred), a type checker should assume that the return type is Self, and it should proceed to evaluate the __init__ method.

…when a class doesn't inherit a `__new__` or `__init__` from a class other than `object`.
…hat returns `Any`. Also clarified what happens when a call to `__call__` or `__new__` evaluates to a union type.
docs/spec/constructors.rst Outdated Show resolved Hide resolved
@erictraut erictraut marked this pull request as ready for review April 5, 2024 05:29
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.

This is an excellent piece of work. I'm approving it as-is -- I would have no objection if it went in unchanged -- but at the same time I have picked a bunch of nits and asked a few questions that you might consider to improve the readability of the spec slightly.

docs/spec/constructors.rst Outdated Show resolved Hide resolved
docs/spec/constructors.rst Outdated Show resolved Hide resolved
docs/spec/constructors.rst Outdated Show resolved Hide resolved
def __new__(cls) -> Any:
return 0

# The __init__ method will not be called in this case, so
Copy link
Member

Choose a reason for hiding this comment

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

s/will/may

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure I understand what you mean, but I rephrased the comment for clarity.

docs/spec/constructors.rst Outdated Show resolved Hide resolved
docs/spec/constructors.rst Outdated Show resolved Hide resolved
Comment on lines +309 to +312
As discussed above, if a class is generic and not explicitly specialized, its
type variables should be solved using the arguments passed to the ``__new__``
and ``__init__`` methods. If one or more type variables are not solved during
these method evaluations, they should take on their default values.
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 this answers a question I had above. Maybe there could be a forward reference planted in the text there? (The text of the rules around specialization is rather long and I found it a bit hard to follow -- maybe this could be explained in depth here, leaving the earlier text shorter?)

docs/spec/constructors.rst Show resolved Hide resolved
docs/spec/constructors.rst Show resolved Hide resolved
docs/spec/constructors.rst Outdated Show resolved Hide resolved
@gvanrossum
Copy link
Member

Thanks! All good now.

@erictraut
Copy link
Collaborator Author

All members of the TC have signed off on this change, so I'm going to merge it.

@erictraut erictraut merged commit 1c3b7df into python:main Apr 11, 2024
4 checks passed
@erictraut erictraut deleted the constructors_spec branch April 11, 2024 00:42
JelleZijlstra added a commit to JelleZijlstra/typing that referenced this pull request Apr 11, 2024
* Added draft chapter to typing spec for constructors.

* Update docs/spec/constructors.rst

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>

* Update docs/spec/constructors.rst

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>

* Update docs/spec/constructors.rst

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>

* Added section on signature consistency between `__new__` and `__init__`.

* Incorporated PR feedback.

* Added clarification based on question in forum.

* Incorporated feedback about callable conversion. Clarified behaviors when a class doesn't inherit a `__new__` or `__init__` from a class other than `object`.

* Tweaked the spec based on Jelle's feedback about a `__new__` method that returns `Any`. Also clarified what happens when a call to `__call__` or `__new__` evaluates to a union type.

* Updated handling of `Any` return types for `__call__` and `__new__` methods to reflect suggestion from @rchen152 in [this post](https://discuss.python.org/t/draft-typing-spec-chapter-for-constructors/49744/22).

* Incorporated feedback from @gvanrossum.

---------

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants