[proxy] web.archive.org — ← back | site home | direct (HTTPS) ↗ | proxy home | ◑ dark◐ light

/ cpython Public

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

Jump to bottom

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

Merged
merged 8 commits into from Dec 8, 2019
Merged

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

merged 8 commits into from Dec 8, 2019

Conversation

Copy link
Contributor

elenaoat commented Nov 3, 2019 •
edited by bedevere-bot

tirkarthi reviewed Nov 3, 2019
View changes
Lib/unittest/mock.py Outdated Show resolved Hide resolved
Copy link

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 changed the title Add check for the type of target bpo-38669: Add check for the type of target Nov 3, 2019
elenaoat force-pushed the bpo-38669 branch 2 times, most recently from 76ed816 to 382d0bc Compare Nov 4, 2019
mariocj89 reviewed Nov 4, 2019
View changes
Copy link
Contributor

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 added the type-feature label Nov 4, 2019
brandtbucher requested changes Nov 5, 2019
View changes
Copy link
Member

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
Misc/NEWS.d/next/Tests/2019-11-04-02-54-16.bpo-38669.pazXZ8.rst Outdated Show resolved Hide resolved
Lib/unittest/mock.py Outdated Show resolved Hide resolved
brandtbucher approved these changes Nov 6, 2019
View changes
Copy link
Member

brandtbucher left a comment

Looks good!

terryjreedy approved these changes Nov 8, 2019
View changes
Copy link
Member

terryjreedy left a comment

This looks ready to commit to me.

PEP8 improvements
d0abbe0 
No need to use f-string.
tirkarthi approved these changes Nov 10, 2019
View changes
Copy link
Member

tirkarthi left a comment

LGTM. Thanks @elenaoat

Copy link

tirkarthi commented Dec 7, 2019

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

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

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 merged commit cd90a52 into python:master Dec 8, 2019
4 checks passed
Copy link

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.
🐍🍒⛏🤖

Copy link

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.
🐍🍒⛏🤖

Copy link

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.

Copy link

cjw296 commented Dec 8, 2019

@Mariatta - any ideas on the socket error above?

Copy link

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
Reviewers

ammaraskar

mariocj89

ZackerySpytz

cjw296

tirkarthi

terryjreedy

brandtbucher

Assignees
No one assigned
Labels
CLA signed needs backport to 3.7 needs backport to 3.8 type-feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet