-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Properties & Incompatible Types in Assignment #4176
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! I have some suggested improvements.
docs/source/common_issues.rst
Outdated
Properties & Incompatible Types in Assignment | ||
--------------------------------------------- | ||
|
||
Consider the code below: |
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 the whole thing is a bit too verbose. You should start with something like "The type of a property is taken from the getter method." Then the example should be more compact (compare some of the other sections in this file). The method bodies could just be ...
and the "main" section shouldn't have if __name__...
.
docs/source/common_issues.rst
Outdated
if __name__ == '__main__': | ||
a = A() | ||
a.foo = 123 | ||
a.foo = 456.789 |
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.
Add a comment here like "# error here".
docs/source/common_issues.rst
Outdated
|
||
error: Incompatible types in assignment (expression has type "float", variable has type "int") | ||
|
||
While overloading class property setters in Python is valid, Mypy infers the type from the initial ``@property`` definition (e.g., ``int``) as the final type for the attribute, regardless of what the ``@property.setter`` declares (e.g., ``Union[int, float]``). This error may be silenced with `# type: ignore <http://mypy.readthedocs.io/en/latest/common_issues.html#spurious-errors-and-locally-silencing-the-checker>`_ |
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.
Please break this line so each line is under 80 characters, and remove the remark about silencing it with # type: ignore
-- by the time a user gets here they should know they can silence anything that way.
Also, "mypy" is not capitalized except at the start of a sentence.
I thought the section "Common issues" is not about known issues (i.e. bugs), but for things that are either correct but unintuitive for some developers (like invariants of Do we really need this? It looks just like a duplicate of the issue. |
@ilevkivskyi , #4167 is currently being considered as a low-priority feature request and not a bug. IMHO, I think this documentation for the current state of mypy is useful for others who may stumble upon issues with properties, as it may change their approach to class architecture. |
Hm, I agree with Ivan that the bar for inclusion of something in the common_issues page should be higher than "low priority feature request" (else half of the issues in the tracker would deserve inclusion here). So I'll close the PR now without merging. If we see an uptick in reports of users who are confused by this we can reconsider. |
Documentation for #4167