Repository navigation
bpo-29573: Fixed unlinking already removed NamedTemporaryFile. - #134
andrewnester wants to merge 1 commit into
Conversation
Sorry, something went wrong.
briancurtin
left a comment
There was a problem hiding this comment.
Looks fine to me, just needs a small test change.
Sorry, something went wrong.
There was a problem hiding this comment.
Should add a self.addCleanup on that tmpdir—see the test above (test_bad_mode) for an example, just to make sure that if this test ever does fail that we don't have a temp dir that doesn't get cleaned up by us.
Sorry, something went wrong.
There was a problem hiding this comment.
@briancurtin right, thanks! fixed
Sorry, something went wrong.
There was a problem hiding this comment.
@briancurtin could you please review it again, it's fixed now
Sorry, something went wrong.
| tmpdir = tempfile.mkdtemp() | ||
| self.addCleanup(support.rmtree, tmpdir) | ||
| with tempfile.NamedTemporaryFile(delete=True, dir=tmpdir) as fp: | ||
| os.system('rm {}'.format(fp.name)) |
There was a problem hiding this comment.
Why does this use os.system to call rm, rather than calling os.unlink? 'rm' is not exactly cross-platform. This test will also fail on Windows (and other systems where you can't delete files that are currently in use). It'll have to be skipped on those platforms.
Sorry, something went wrong.
Replace the direct access to PyObject members with the preferred macros.
|
To try and help move older pull requests forward, we are going through and backfilling 'awaiting' labels on pull requests that are lacking the label. Based on the current reviews, the best we can tell in an automated fashion is that a core developer requested changes to be made to this pull request. If/when the requested changes have been made, please leave a comment that says, I have made the requested changes; please review again. That will trigger a bot to flag this pull request as ready for a follow-up review. |
Sorry, something went wrong.
Replace the direct access to PyObject members with the preferred macros. (cherry picked from commit c7dcca5)
|
@briancurtin @Yhg1s @brettcannon Is this PR worth being revived? The user's repo does not exist any more. I could take care of it by creating a new PR if you want |
Sorry, something went wrong.
|
@flavianh up to you if you want to create a new PR to try and solve the problem. |
Sorry, something went wrong.
Fix for http://bugs.python.org/issue29573
https://bugs.python.org/issue29573