Skip to content

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

Merged
cjw296 merged 8 commits into
python:masterfrom
elenaoat:bpo-38669
Dec 8, 2019
Merged

bpo-38669: Add check for the type of target#17034
cjw296 merged 8 commits into
python:masterfrom
elenaoat:bpo-38669

Conversation

@elenaoat

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

Copy link
Copy Markdown
Contributor

tirkarthi
tirkarthi reviewed Nov 3, 2019
View reviewed changes
Comment thread Lib/unittest/mock.py Outdated
@tirkarthi

tirkarthi commented Nov 3, 2019

Copy link
Copy Markdown
Member

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 November 4, 2019 02:50
ammaraskar
ammaraskar reviewed Nov 4, 2019
View reviewed changes
Comment thread Lib/unittest/test/testmock/testpatch.py Outdated
mariocj89
mariocj89 reviewed Nov 4, 2019
View reviewed changes

@mariocj89 mariocj89 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 !

Comment thread Lib/unittest/mock.py Outdated
Comment thread Lib/unittest/mock.py Outdated
@brettcannon brettcannon added the type-feature A feature request or enhancement label Nov 4, 2019
ammaraskar
ammaraskar reviewed Nov 5, 2019
View reviewed changes
Comment thread Lib/unittest/test/testmock/testpatch.py Outdated
brandtbucher
brandtbucher requested changes Nov 5, 2019
View reviewed changes

@brandtbucher brandtbucher left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Comment thread Lib/unittest/mock.py Outdated
Comment thread Misc/NEWS.d/next/Tests/2019-11-04-02-54-16.bpo-38669.pazXZ8.rst Outdated
Comment thread Lib/unittest/mock.py Outdated
brandtbucher
brandtbucher approved these changes Nov 6, 2019
View reviewed changes

@brandtbucher brandtbucher left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good!

terryjreedy
terryjreedy approved these changes Nov 8, 2019
View reviewed changes

@terryjreedy terryjreedy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks ready to commit to me.

ZackerySpytz
ZackerySpytz reviewed Nov 8, 2019
View reviewed changes
Comment thread Lib/unittest/test/testmock/testpatch.py Outdated
PEP8 improvements
d0abbe0 
No need to use f-string.
tirkarthi
tirkarthi approved these changes Nov 10, 2019
View reviewed changes

@tirkarthi tirkarthi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Thanks @elenaoat

@tirkarthi

tirkarthi commented Dec 7, 2019

Copy link
Copy Markdown
Member

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

cjw296
cjw296 approved these changes Dec 8, 2019
View reviewed changes

@cjw296 cjw296 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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
@miss-islington

miss-islington commented Dec 8, 2019

Copy link
Copy Markdown
Contributor

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

@miss-islington

miss-islington commented Dec 8, 2019

Copy link
Copy Markdown
Contributor

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

@miss-islington

miss-islington commented Dec 8, 2019

Copy link
Copy Markdown
Contributor

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

cjw296 commented Dec 8, 2019

Copy link
Copy Markdown
Contributor

@Mariatta - any ideas on the socket error above?

@miss-islington

miss-islington commented Dec 8, 2019

Copy link
Copy Markdown
Contributor

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 ammaraskar ammaraskar approved these changes
@cjw296 cjw296 cjw296 approved these changes
@tirkarthi tirkarthi tirkarthi approved these changes
@terryjreedy terryjreedy terryjreedy approved these changes
@brandtbucher brandtbucher brandtbucher approved these changes
+2 more reviewers
@mariocj89 mariocj89 mariocj89 left review comments
@ZackerySpytz ZackerySpytz ZackerySpytz left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

type-feature A feature request or enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.