Move shutil patches to fake shutil module #1182
Conversation
mrbean-bremen
force-pushed
the
shutil_patches
branch
from
bdcabc0 to
afd635c
Compare
- allows to use FakeShutilModule outside of Patcher
mrbean-bremen
force-pushed
the
shutil_patches
branch
from
afd635c to
a20e10f
Compare
|
|
||
| @contextlib.contextmanager | ||
| def patch_global_vars(self): | ||
| self.start_patching_global_vars() |
There was a problem hiding this comment.
Shouldn't it be thread-safe? (adding locking) That is exactly the problem I described in the issue, which I solve not by patching but by injection. I also thought about such patching, but I didn't like the idea of locking, but since you made such a decision yourself in the code, maybe you should add it?
There was a problem hiding this comment.
Darn - one time I don't wait with the merge, and somebody is actually reviewing...
Thanks for the suggestion - I think you are right. While pyfakefs is not designed to be thread-safe (as recently discussed in another thread), it makes sense to add a lock in this place. Will do this probably tonight...
| fake_shutil = fake_filesystem_shutil.FakeShutilModule(filesystem) | ||
|
|
||
| with patch("os", fake_os): | ||
| with patch("shutil.os", shutil_mock): |
There was a problem hiding this comment.
And here
I still can't understand this example
What is shutil_mock, and why we replace shutil.>>os<< with it?
There was a problem hiding this comment.
Well, you can't understand it, because it is obviously a typo...
I always see this stuff in code written by others, never in my own.
Next time I will certainly wait for a review.
There was a problem hiding this comment.
Actually I think I'll just revert this documentation change, as there is currently there is no good way to support the standalone version. This has to wait for a better fix.
Or probably revert all of the PR - this now looks quite bogus to me. I'll have a better look another day, don't have much time now.
There was a problem hiding this comment.
Are you planning to revert only the usage example, not the new functionality itself? I just tested the library version from the main branch with my current product tests—no errors found
There was a problem hiding this comment.
Well, the problem is that I made an embarassing typo ("start" instead of "stop") that led to the patches not to be reverted. This may not be a problem if you only want to use the patched version as in your case, but is if afterwards the real code should be executed. It would not be noticable either, because the patches just change the code path so that some optimized variants are not used that cannot easily by patched, but this makes potential subtle problems harder to detect.
Anyway, I cannot release the code as is, and after fixing the typo, the tests do not pass in the CI under macOS. Unfortunately, I cannot test this locally, so it may take a while to resolve this. Also, I'm short on time this week (and possibly the next), and I also need a bit more sleep to avoid such dumb mistakes.
TL;DR - it is unlikely that I will finish this soon, certainly not before the next weekend.
There was a problem hiding this comment.
Thank you, I'm absolutely not rushing you, and I didn't expect you to resolve this issue so fast.
I understand this is open source, so even if it doesn't get resolved - no problem at all
Tasks