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-35512: Resolve string target to patch.dict decorator during function call #12000

Merged
merged 5 commits into from Feb 24, 2019

Conversation

Projects
None yet
6 participants
@tirkarthi
Copy link
Contributor

commented Feb 23, 2019

String target provided to patch.dict can be reassigned after the decorator decorates the function. So resolve the target before function call to ensure the new reference is patched instead of patching the old reference stored in the constructor.

https://bugs.python.org/issue35512

@tirkarthi

This comment has been minimized.

Copy link
Contributor Author

commented Feb 23, 2019

@mariocj89 I have just moved the resolution to _patch_dict since I felt storing the target in in_dict_name is little misleading since it's not always a name. Also that using a separate variable for only string target felt little verbose to me since it's essentially the same thing.

For tests I have used testmock.support to have a dictionary that I am trying to patch as a string target in testpatch.py which patches the older reference without the PR. Let me know if this could be made better and comments/NEWS need to be reworded.

Thanks

@mariocj89
Copy link
Contributor

left a comment

I think it is a little bit weird that self.in_dict changes when executing the function/ctx manager but given that we are using it just as a way to make it available across functions LGTM.

@tirkarthi

This comment has been minimized.

Copy link
Contributor Author

commented Feb 24, 2019

Thanks for the review Mario. I would wait for feedback from @jaraco and @cjw296 to see if this can be merged.

self.assertEqual(support.target['bar'], 'BAR')

support.target = {'foo': 'BAZ'}
test()

This comment has been minimized.

Copy link
@cjw296

cjw296 Feb 24, 2019

Contributor

I'd expect this test() call to be wrapped in a try/finally to put support.target back like it was, regardless of what happens in test.

This comment has been minimized.

Copy link
@tirkarthi

tirkarthi Feb 24, 2019

Author Contributor

Yes, tests might use the variable in the future. Good catch. Thanks.

test()

self.assertEqual(support.target['foo'], 'BAZ')
self.assertNotIn('bar', support.target)

This comment has been minimized.

Copy link
@cjw296

cjw296 Feb 24, 2019

Contributor

Why not just:
self.assertEqual(support.target, {'foo': 'BAZ'})
?
(I guess the same goes for the assertions inside test too?)

@@ -1620,8 +1620,6 @@ class _patch_dict(object):
"""

def __init__(self, in_dict, values=(), clear=False, **kwargs):
if isinstance(in_dict, str):
in_dict = _importer(in_dict)

This comment has been minimized.

Copy link
@cjw296

cjw296 Feb 24, 2019

Contributor

Is the setting of self.in_dict on the line below still needed?

This comment has been minimized.

Copy link
@tirkarthi

tirkarthi Feb 24, 2019

Author Contributor
@cjw296

cjw296 approved these changes Feb 24, 2019

Copy link
Contributor

left a comment

LGTM!


try:
support.target = {'foo': 'BAZ'}
test()

This comment has been minimized.

Copy link
@cjw296

cjw296 Feb 24, 2019

Contributor

You could always add the following here, so be super sure:
self.assertEqual(support.target, {'foo': 'BAZ'})

This comment has been minimized.

Copy link
@tirkarthi

tirkarthi Feb 24, 2019

Author Contributor

Sure, added with d7f5c11

@cjw296 cjw296 merged commit a875ea5 into python:master Feb 24, 2019

5 checks passed

Azure Pipelines PR #20190224.20 succeeded
Details
bedevere/issue-number Issue number 35512 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@miss-islington

This comment has been minimized.

Copy link

commented Feb 24, 2019

Thanks @tirkarthi 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!

@tirkarthi

This comment has been minimized.

Copy link
Contributor Author

commented Feb 24, 2019

Thanks @cjw296 for merging this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.