|
@mariocj89 I have just moved the resolution to For tests I have used Thanks |
There was a problem hiding this 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.
| self.assertEqual(support.target['bar'], 'BAR') | ||
|
|
||
| support.target = {'foo': 'BAZ'} | ||
| test() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes, tests might use the variable in the future. Good catch. Thanks.
| test() | ||
|
|
||
| self.assertEqual(support.target['foo'], 'BAZ') | ||
| self.assertNotIn('bar', support.target) |
There was a problem hiding this comment.
Why not just:
self.assertEqual(support.target, {'foo': 'BAZ'})
?
(I guess the same goes for the assertions inside test too?)
|
|
||
| def __init__(self, in_dict, values=(), clear=False, **kwargs): | ||
| if isinstance(in_dict, str): | ||
| in_dict = _importer(in_dict) |
There was a problem hiding this comment.
Is the setting of self.in_dict on the line below still needed?
There was a problem hiding this comment.
I think it's needed since it's self.dict is passed in https://github.com/python/cpython/pull/12000/files/f2abf2667ca660f11db62bb4038529ada8d52925#diff-ff75b1b83c21770847ade91fa5bb2525L1652 and other places in the class.
|
|
||
| try: | ||
| support.target = {'foo': 'BAZ'} | ||
| test() |
There was a problem hiding this comment.
You could always add the following here, so be super sure:
self.assertEqual(support.target, {'foo': 'BAZ'})
|
Thanks @tirkarthi for the PR, and @cjw296 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7. |
|
Thanks @cjw296 for merging this :) |
String target provided to
patch.dictcan 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