-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
gh-109543: Remove unnecessary hasattr check #109544
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
Conversation
Also added a new test case covering the scenario I thought this might be about.
class TD2(TD1): | ||
b: str | ||
|
||
self.assertIs(TD2.__total__, True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know you're not changing behaviour here, but are the precise semantics here specified anywhere? Mypy has some interesting behaviour here -- it treats all the keys present in TD2
as required, but all the keys present in TD1
as not required. That implies to me that TD2
should not be seen as a "total" TypedDict
.
https://mypy-play.net/?mypy=latest&python=3.11&gist=e7ef88dd8c2b5697297c4739d471ac45
The __total__
attribute seems to be pretty meaningless these days, though, in a post-PEP-655 world:
>>> from typing import *
>>> class Foo(TypedDict):
... x: Required[int]
... y: NotRequired[str]
...
>>> Foo.__total__
True
Maybe we should treat that as a bug and make it meaningful. Or maybe we should just be clearer in the documentation that __total__
doesn't indicate whether or not all keys in the TypedDict
are required or not, it just literally indicates whether that specific TypedDict
was constructed using total=True
(but I'm not sure why that would be useful to anybody).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mypy behavior is right; that was the way to mix Required and NotRequired keys before PEP 655.
Agree that __total__
isn't that useful and you should generally look at __required_keys__
and __optional_keys__
. However, I think the current behavior is right. If you do want to use __total__
for introspection, you should look not just at the current class's attribute, but also those of base classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's strange to me that we iterate through the base classes as part of the class construction in order to make sure we have accurate __required_keys__
, __optional_keys__
and __annotations__
attributes on the produced class, but we don't do the same for the __total__
attribute. (We probably shouldn't be doing that for the __annotations__
attribute, since that's not the way __annotations__
works on any other class... but we do it anyway.) It seems very inconsistent to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Submitted #109547 with some enhancements to the docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The semantics of this attribute seem pretty confusing -- I'm honestly wondering whether it might not be better to deprecate it. But I'm pretty confident you're correct that there's no way for __total__
to already be set at this point.
As discussed in comments to python#109544, the semantics of this attribute are somewhat confusing. Add a note explaining its limitations and steering users towards __required_keys__ and __optional_keys__ instead.
|
|
|
|
|
|
As discussed in comments to #109544, the semantics of this attribute are somewhat confusing. Add a note explaining its limitations and steering users towards __required_keys__ and __optional_keys__ instead.
As discussed in comments to pythonGH-109544, the semantics of this attribute are somewhat confusing. Add a note explaining its limitations and steering users towards __required_keys__ and __optional_keys__ instead. (cherry picked from commit f49958c) Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
As discussed in comments to pythonGH-109544, the semantics of this attribute are somewhat confusing. Add a note explaining its limitations and steering users towards __required_keys__ and __optional_keys__ instead. (cherry picked from commit f49958c) Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
… (#109983) As discussed in comments to GH-109544, the semantics of this attribute are somewhat confusing. Add a note explaining its limitations and steering users towards __required_keys__ and __optional_keys__ instead. (cherry picked from commit f49958c) Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Also added a new test case covering the scenario I thought this might be about.
As discussed in comments to python#109544, the semantics of this attribute are somewhat confusing. Add a note explaining its limitations and steering users towards __required_keys__ and __optional_keys__ instead.
… (#109982) Enhance TypedDict docs around required/optional keys (GH-109547) As discussed in comments to GH-109544, the semantics of this attribute are somewhat confusing. Add a note explaining its limitations and steering users towards __required_keys__ and __optional_keys__ instead. (cherry picked from commit f49958c) Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Also added a new test case covering the scenario I thought this might be about.
As discussed in comments to python#109544, the semantics of this attribute are somewhat confusing. Add a note explaining its limitations and steering users towards __required_keys__ and __optional_keys__ instead.
In relation to python#109544 which changed this behavior. Signed-off-by: Daniel Sperber <github.blurry@9ox.net>
In relation to python#109544 which changed this behavior. Signed-off-by: Daniel Sperber <github.blurry@9ox.net>
In relation to python#109544 which changed this behavior. Signed-off-by: Daniel Sperber <github.blurry@9ox.net>
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>
…re is an assignment in the class body. (pythonGH-130460) In relation to pythonGH-109544 which changed this behavior. (cherry picked from commit d8ce092) Co-authored-by: Daraan <github.blurry@9ox.net> Signed-off-by: Daniel Sperber <github.blurry@9ox.net>
…hen there is an assignment in the class body. (GH-130460) (#130462) Add test checking value of a TypedDict's __total__ attribute when there is an assignment in the class body. (GH-130460) In relation to GH-109544 which changed this behavior. (cherry picked from commit d8ce092) Signed-off-by: Daniel Sperber <github.blurry@9ox.net> Co-authored-by: Daraan <github.blurry@9ox.net>
Also added a new test case covering the scenario I thought this
might be about.