Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Fixed bug with NamedTemporaryFile on Windows#4

Merged
c-w merged 2 commits into
ascoderu:masterascoderu/xtarfile:masterfrom
mrmichaeladavis:mastermrmichaeladavis/xtarfile:masterCopy head branch name to clipboard
May 31, 2020
Merged

Fixed bug with NamedTemporaryFile on Windows#4
c-w merged 2 commits into
ascoderu:masterascoderu/xtarfile:masterfrom
mrmichaeladavis:mastermrmichaeladavis/xtarfile:masterCopy head branch name to clipboard

Conversation

@mrmichaeladavis
Copy link
Copy Markdown
Contributor

Changed NamedTemporaryFile to be manually deleted isntead of automatic. We delete it manually because otherwise on Windows it gets deleted before we the temp file is saved to the output file location. This is because on Windows, file handles with the O_TEMPORARY flag (which is set if we pass delete=True) are deleted as soon as they're closed.

See the docs at https://docs.python.org/3/library/tempfile.html#tempfile.NamedTemporaryFile

"Whether the name can be used to open the file a second time, while the named temporary file is still open, varies across platforms (it can be so used on Unix; it cannot on Windows NT or later). If delete is true (the default), the file is deleted as soon as it is closed. The returned object is always a file-like object whose file attribute is the underlying true file object. This file-like object can be used in a with statement, just like a normal file."

Changed NamedTemporaryFile to be manually deleted isntead of automatic. We delete it manually because otherwise on Windows it gets deleted before we the temp file is saved to the output file location. This is because on Windows, file handles with the O_TEMPORARY flag (which is set if we pass `delete=True`) are deleted as soon as they're closed.
@c-w c-w self-assigned this May 30, 2020
Copy link
Copy Markdown
Member

@c-w c-w left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution and fix. I made a follow-up task to also run the CI on Windows (e.g. via Github Actions) so that we can spot these sorts of issues in the future (see #5).

I see that the CI is currently failing (just flake8 complaining about line length so should be quick to fix). Could you take care of this and then I'd say this is ready for merge.

Copy link
Copy Markdown
Member

@c-w c-w left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just pushed a change to fix the formatting so this is ready to go. Thanks so much for your contribution!

@c-w c-w merged commit bed3f8a into ascoderu:master May 31, 2020
@c-w
Copy link
Copy Markdown
Member

c-w commented May 31, 2020

This fix has been released in version 0.0.4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.