FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Allow covariance for read-only attributes by TH3CHARLie · Pull Request #8350 · python/mypy · GitHub

  1. python
  2. mypy

Repository navigation

Allow covariance for read-only attributes - #8350

Merged
msullivan merged 7 commits into
python:masterfrom
TH3CHARLie:fix-5836
Feb 7, 2020
ConversationCommits (7)ChecksFiles changed
Merged

Allow covariance for read-only attributes#8350
msullivan merged 7 commits into
python:masterfrom
TH3CHARLie:fix-5836

Conversation

TH3CHARLie commented Jan 31, 2020 •
edited
Loading

Copy link
Copy Markdown
Collaborator

resolves #5836

For now, I've not added any tests yet. I've tested against code samples in #5836 and #3260 and the patch is working but adding either of them seems to require modifying a lot in test fixtures

The new helper function is_writable_attribute is actually borrowed from check_no_writable. I'd also like to refactor the latter function's name and implement it using the new helper in this PR, since I think its name is kind of misleading considering the comments

mypy/mypy/checker.py

Lines 2391 to 2402 in ea3c65c

def check_no_writable(self, name: str, base_node: Optional[Node], ctx: Context) -> None:
"""Check that a final variable doesn't override writeable attribute.
This is done to prevent situations like this:
class C:
attr = 1
class D(C):
attr: Final = 2
x: C = D()
x.attr = 3 # Oops!
"""

JukkaL commented Feb 3, 2020

Copy link
Copy Markdown
Collaborator

but adding either of them seems to require modifying a lot in test fixtures

What's missing in text fixtures? Maybe you can simplify the examples to not rely on so many features.

TH3CHARLie commented Feb 3, 2020 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

but adding either of them seems to require modifying a lot in test fixtures

What's missing in text fixtures? Maybe you can simplify the examples to not rely on so many features.

Good point. I've simplified the case from #5836 and added it. I've also done some refactor work to reuse the new helper added and modify the function's name to make it less misleading. But it is not the main focus of the PR and I'd like to know your thoughts on it. @JukkaL

msullivan approved these changes Feb 7, 2020
View reviewed changes

msullivan left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

OK this seems like a reasonable improvement. I think this doesn't affect all attributes. Regular attributes we just always allow covariant overriding of, even when it is totally wrong.

msullivan merged commit 3bd6e47 into python:master Feb 7, 2020
TH3CHARLie deleted the fix-5836 branch February 8, 2020 08:56
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

msullivan msullivan approved these changes
ilevkivskyi Awaiting requested review from ilevkivskyi
JukkaL Awaiting requested review from JukkaL

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Decorated method type-incompatibility with superclass

3 participants

Back | FazBrowse Home | New Git URL