Right, the problem with target_temp_dir
is that it's created conditionally - so sometimes it's a context manager, and other times it's None
. So we can't "just" enter it earlier, as you can't do with None
.
When we drop Python 2, we could fix this with contextlib.ExitStack
. This isn't worth vendoring the contextlib
backport just for Python 2, though. We could also write our own MaybeContextManager
class that wraps an optional context manager, but again that's a lot of work.
I'm going to make the call that this should be handled in a follow-up PR, and for now just allow left-over temp directories in the test.
_Originally posted by @pfmoore in https://github.com/pypa/pip/pull/8462#issuecomment-649349135_
When we drop Python 2, we could fix this with
contextlib.ExitStack
Hi, FYI pip is already vendoring contextlib2
.
Ooh, I didn't know that! Thanks for the pointer. In which case, I'll take a look at using ExitStack for this.
And actually, that led me to CommandContextMixIn
and the fact that all of our commands have a built in exit stack - which is really cool, and makes fixing this a lot easier than I thought. I hope - just running the tests now 馃檪
Thanks!
@pfmoore did you fix this yet? ;)
No, I think I dropped the ball on this one. If I had a branch with a fix in, I'm not sure where it's gone 馃檨
@pfmoore, I think you've already fixed it in f1622363606654ca18e27bbcfd13f56f6f40014b :smile:
Gosh, I knew I鈥檝e seen that CommandContextMixIn
implementation before. I thought I was hallucinating or something.
@pfmoore, I think you've already fixed it in f162236 馃槃
I thought it rang a bell 馃槃
Too many issues in the tracker, not enough cells in my brain...