Skip to content
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

bpo-38669: Add check for the type of target #17034

Merged
merged 8 commits into from Dec 8, 2019
Merged

Conversation

elenaoat
Copy link
Contributor

@elenaoat elenaoat commented Nov 3, 2019

Lib/unittest/mock.py Outdated Show resolved Hide resolved
@tirkarthi
Copy link

@tirkarthi tirkarthi commented Nov 3, 2019

Please add the issue number in the required format so that the PR is linked to the issue. I guess this needs a NEWS entry.

@elenaoat elenaoat changed the title Add check for the type of target bpo-38669: Add check for the type of target Nov 3, 2019
@elenaoat elenaoat force-pushed the bpo-38669 branch 2 times, most recently from 76ed816 to 382d0bc Compare Nov 4, 2019
Copy link
Contributor

@mariocj89 mariocj89 left a comment

Two small comments, the patch will be quite helpful as I've seen many people confused when using mock.patch.dict because of exactly this. Thanks a lot @elenaoat !

Lib/unittest/mock.py Outdated Show resolved Hide resolved
Lib/unittest/mock.py Outdated Show resolved Hide resolved
@brettcannon brettcannon added the type-feature label Nov 4, 2019
Copy link
Member

@brandtbucher brandtbucher left a comment

Thanks for the patch @elenaoat! I've got a couple of formatting suggestions, but otherwise I like what you've done here:

Lib/unittest/mock.py Outdated Show resolved Hide resolved
Lib/unittest/mock.py Outdated Show resolved Hide resolved
Copy link
Member

@brandtbucher brandtbucher left a comment

Looks good!

Copy link
Member

@terryjreedy terryjreedy left a comment

This looks ready to commit to me.

No need to use f-string.
Copy link
Member

@tirkarthi tirkarthi left a comment

LGTM. Thanks @elenaoat

@tirkarthi
Copy link

@tirkarthi tirkarthi commented Dec 7, 2019

@cjw296 Given the approvals it would be helpful to have your review of this PR. Thanks.

cjw296
cjw296 approved these changes Dec 8, 2019
Copy link
Contributor

@cjw296 cjw296 left a comment

Looks good to me! Honestly, I'd think of this as a bug and so would be happy to see it backported.

@cjw296 cjw296 merged commit cd90a52 into python:master Dec 8, 2019
4 checks passed
@miss-islington
Copy link

@miss-islington miss-islington commented Dec 8, 2019

Thanks @elenaoat for the PR, and @cjw296 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒🤖

@miss-islington
Copy link

@miss-islington miss-islington commented Dec 8, 2019

Thanks @elenaoat for the PR, and @cjw296 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒🤖

@miss-islington
Copy link

@miss-islington miss-islington commented Dec 8, 2019

I'm having trouble backporting to 3.7. Reason: 'Error 110 while writing to socket. Connection timed out.'. Please retry by removing and re-adding the needs backport to 3.7 label.

@cjw296
Copy link

@cjw296 cjw296 commented Dec 8, 2019

@Mariatta - any ideas on the socket error above?

@miss-islington
Copy link

@miss-islington miss-islington commented Dec 8, 2019

Thanks @elenaoat for the PR, and @cjw296 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒🤖 I'm not a witch! I'm not a witch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet