Skip to content

Support or in isinstance checks - #1712

Merged
JukkaL merged 2 commits into
python:masterfrom
rwbarton:rwbarton-isinstance
Jun 27, 2016
Merged

JukkaL merged 2 commits into
python:masterfrom
rwbarton:rwbarton-isinstance

Conversation

@rwbarton

Copy link
Copy Markdown
Contributor

Fixes #942.

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")

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.

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

@rwbarton

Copy link
Copy Markdown
Contributor Author

Done (and rebased).

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

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.

I suggest revealing the type of x here.

@JukkaL

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
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.
@rwbarton
rwbarton force-pushed the rwbarton-isinstance branch from d025588 to 56ce18d Compare June 22, 2016 20:47
@rwbarton

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
JukkaL merged commit 32062eb into python:master Jun 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants