-
Notifications
You must be signed in to change notification settings - Fork 243
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
Update conformance test sources to initialize uninitialized variables #1804
Conversation
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.
Thanks for this change. Most of it looks good to me, but there are a few places where the introduction of an __init__
method is inappropriate because it changes what the test case was attempting to test.
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.
LGTM
@yangdanny97, this draft of the PR looks good to me. If you think it's ready to go, I'm happy to merge it. |
Sounds good, thanks! |
@@ -88,6 +88,9 @@ def int(self) -> None: # OK | |||
|
|||
y: int = 0 # E: Refers to local int, which isn't a legal type expression | |||
|
|||
def __init__(self, ClassC: "ClassC") -> None: |
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 think this is at least arguably incorrect, since ClassC
in the annotation should resolve to the class variable ClassC
, not the global. Mypy gives an error for this case.
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.
This looks to me like a bug in mypy. This is a deferred evaluation because the annotation is in quotes. The ClassC
declared in the class body isn't in scope when the evaluation is performed.
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.
That said, this test isn't intended to check for this behavior, so we could trivially change it to avoid the mypy bug:
def __init__(self) -> None:
self.ClassC = ClassC()
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.
@yangdanny97, if you're OK with this proposed modification, please push an update.
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.
@JelleZijlstra, are you still reviewing the PR, or are you done? If you're good with it, please merge.
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.
Merged!
Summary: per Eric's comment in python/typing#1804 (comment) _value_ is magic for enums, and we shouldn't give uninitialized attr errors for it Reviewed By: migeed-z Differential Revision: D59833300 fbshipit-source-id: 59391ee56aecbab8b648648d27927258b2b6d3fc
This PR adds constructors that initialize the attributes for the classes defined in each conformance test, per @erictraut comment on another PR (linked below).
#1734 (comment)
Please let me know if you'd prefer things to be initialized in a different way.
cc @stroxler