Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Mitigate a regression in the external context #6006

Merged
merged 1 commit into from
Dec 5, 2018

Conversation

ilevkivskyi
Copy link
Member

Mitigates #5738 by reverting heuristics for type variable return vs instance type outer context changed in #5699. Both versions are kind of unprincipled, but the old one seems to work better in real codebases.

This doesn't touch the core fixes in #5699

cc @euresti could you please double check this also fixes your attr.Factory use cases?

@ilevkivskyi ilevkivskyi requested a review from JukkaL December 4, 2018 18:24
@ilevkivskyi
Copy link
Member Author

The AppVeyor failure is a flake mypy.ipc.IPCException: The NamedPipe at \\.\pipe\dmypy-test-ipc.sock-b'7Avf//Ea'.pipe was not found, cc @ethanhs

Also this passes cleanly on our codebases.

@emmatyping
Copy link
Collaborator

@ilevkivskyi Ah, I think that is likely related to whatever failures already observed in #5983. Sorry!

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

This looks like a reasonable thing to do. The previous behavior was arguably more ad hoc than this one.

@ilevkivskyi ilevkivskyi merged commit 393e9ab into python:master Dec 5, 2018
@ilevkivskyi ilevkivskyi deleted the mitigate-context branch December 5, 2018 15:19
@ilevkivskyi
Copy link
Member Author

@JukkaL Probably it makes sense to cherry-pick this one.

@gvanrossum
Copy link
Member

gvanrossum commented Dec 5, 2018 via email

@JukkaL
Copy link
Collaborator

JukkaL commented Dec 5, 2018

This is a fundamental enough change that we should test this internally before a release.

@gvanrossum
Copy link
Member

gvanrossum commented Dec 5, 2018 via email

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants