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-46534: Implement PEP 673 Self in typing.py #30924

Merged
merged 15 commits into from
Feb 7, 2022

Conversation

Gobot1234
Copy link
Contributor

@Gobot1234 Gobot1234 commented Jan 26, 2022

Tests were copied from typing_extensions

Co-authored-by: Pradeep Kumar Srinivasan <[email protected]>
@JelleZijlstra JelleZijlstra self-requested a review January 26, 2022 18:07
Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your work! 👍

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
Lib/test/test_typing.py Show resolved Hide resolved
Doc/library/typing.rst Outdated Show resolved Hide resolved
@JelleZijlstra
Copy link
Member

Did you mean to push some changes? Doesn't look like our comments were addressed.

@Gobot1234
Copy link
Contributor Author

Did you mean to push some changes? Doesn't look like our comments were addressed.

Sorry my bad.

Copy link
Member

@Fidget-Spinner Fidget-Spinner left a comment

Choose a reason for hiding this comment

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

The runtime behavior LGTM. If Jelle and Nikita (and anyone else interested) are happy with the docs I'll merge. Personally I think the docs are very clear too.

Thank you for implementing this James!

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks again! 🎉

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.

I personally still find the documentation a little sparse, and would love to see more of the stuff from PEP 673 included in the typing module documentation. (It was a really great PEP! Extremely clear and informative, really nicely written — kudos 😀)

Nonetheless, the documentation you've added here looks great, and more can always be added in future PRs.

Congrats on the PEP being accepted! I'm super excited to start using this feature :)

@AlexWaygood
Copy link
Member

Should an entry be added to "What's New in Python 3.11"?

@gvanrossum
Copy link
Member

Should an entry be added to "What's New in Python 3.11"?

Yes.

@JelleZijlstra
Copy link
Member

Should that be done in this PR? I thought the what's new was collated manually later. I assume typing.reveal_type() and friends should also get what's new entries.

@gvanrossum
Copy link
Member

Should that be done in this PR? I thought the what's new was collated manually later. I assume typing.reveal_type() and friends should also get what's new entries.

What's New entries can be added at any time, and often are edited in a later stage. But there's absolutely no rule that says you can't add one at the same time you change the code! We've had a tradition of not requiring What's New for a few reasons: devs writing code aren't always good at writing docs, and vice versa; and sometimes What's New needs to be reorganized to make it more readable for users who want to quickly know what's changed. We especially want to avoid that What's New reads like a Changelog file -- we have the Misc/NEWS entries for that (those can still be edited but it rarely happens unless there are blatant errors, and the goal there is to be complete, just not as detailed as "git log" output).

So it's up to you.

@JelleZijlstra
Copy link
Member

Thanks Guido! For my function PRs, I'd rather add a single PR summarizing all of them in What's New once we have decided on each of the PRs. I'm also happy to add a What's New entry for PEP 673 in that case.

So I think this PR is ready to merge.

@JelleZijlstra
Copy link
Member

@gvanrossum I think this can be merged now; Alex, Nikita, and I approved it. I'll work on What's New for typing in 3.11 once my other PRs are resolved.

@gvanrossum
Copy link
Member

Let's see how the tests fare (tip: when the tests aren't run because the PR submitter is new to the project, ask a core dev to run the tests).

@JelleZijlstra
Copy link
Member

Everything is green here now.

Comment on lines 603 to 604
This annotation is semantically equivalent to using a :class:`TypeVar` with ``bound=Foo`` as
both the return annotation and the annotation for the ``self`` parameter.
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if instead of this complicated sentence we should just show the code (with a comment saying something like "don't do this, do the other thing").

Maybe also explain how this is similar and different than

class Foo:
    def return_self(self) -> "Foo":
        ...

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

Thanks a lot for this @Gobot1234 and @pradeep90! And thanks Jelle for sponsoring.

@Gobot1234
Copy link
Contributor Author

Thank you to everyone that reviewed this.

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.

9 participants