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

Support `or` in `isinstance` checks by rwbarton · Pull Request #1712 · python/mypy · GitHub

  1. python
  2. mypy

Repository navigation

Support or in isinstance checks - #1712

Merged
JukkaL merged 2 commits into
python:masterfrom
rwbarton:rwbarton-isinstance
Jun 27, 2016
ConversationCommits (2)ChecksFiles changed
Merged

Support or in isinstance checks#1712
JukkaL merged 2 commits into
python:masterfrom
rwbarton:rwbarton-isinstance

Conversation

rwbarton commented Jun 15, 2016

Copy link
Copy Markdown
Contributor

Fixes #942.

JukkaL reviewed Jun 15, 2016
View reviewed changes
Comment thread test-data/unit/check-isinstance.test Outdated
[builtins fixtures/isinstance.py]
[out]
main: note: In function "h":
main:13: error: Incompatible return value type (got "object", expected "int")

JukkaL Jun 15, 2016

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

Can you use a # E: comment here to make the test case easier to read?

rwbarton mentioned this pull request Jun 15, 2016
Closed
rwbarton force-pushed the rwbarton-isinstance branch from 172ac66 to d025588 Compare June 16, 2016 02:59

rwbarton commented Jun 16, 2016

Copy link
Copy Markdown
Contributor Author

Done (and rebased).

JukkaL reviewed Jun 17, 2016
View reviewed changes
Comment thread test-data/unit/check-isinstance.test Outdated
flag = 2
def f(x: A) -> int:
if isinstance(x, B) or isinstance(x, C):
return x.flag

JukkaL Jun 17, 2016

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

I suggest revealing the type of x here.

JukkaL commented Jun 17, 2016

Copy link
Copy Markdown
Collaborator

LGTM! Some minor nits about test cases and comments above.

rwbarton added 2 commits June 22, 2016 12:24
Refactor combination of conditional maps
9218536 
There was also a slight logic error in the old version (when
left_if_vars was None but right_if_vars was not, the branch was
treated as reachable). Fix it and add a test for that case.
Support or in isinstance checks
56ce18d 
Fixes python#942.
rwbarton force-pushed the rwbarton-isinstance branch from d025588 to 56ce18d Compare June 22, 2016 20:47

rwbarton commented Jun 22, 2016

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review. I had meant to add more documentation about TypeMap actually, and writing these made me notice that the behavior of isinstance(x, T) and isinstance(x, U) was actually nondeterministic. Pushed a new version addressing this and your other comments.

JukkaL merged commit 32062eb into python:master Jun 27, 2016
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

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL