Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

writer.py: postpone creating the output file #43

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bawr
Copy link
Contributor

@bawr bawr commented Jun 2, 2016

Windows can't unlink a file while it is still open, and raises an exception.
Additionally, self.output_file isn't accessed anywhere else, by any methods.
As such, creation should happen after we make sure that there are no bugs.

Windows can't unlink a file while it is still open, and raises an exception.
Additionally, self.output_file isn't accessed anywhere else, by any methods.
As such, creation should happen *after* we make sure that there are no bugs.
@Maratyszcza
Copy link
Owner

The motivation for creating a file early is to fail fast if the file can't be created (e.g. parent directory doesn't exist or insufficient access rights).
I would accept a fix for the unlinking-on-Windows problem, but without postponing file creation.

@bawr
Copy link
Contributor Author

bawr commented Jun 2, 2016

Right, do you foresee a problem with closing the file early, in the exception branch?

That was basically the other quick fix I considered.

@Maratyszcza
Copy link
Owner

Closing + unlinking? Or just closing?

@bawr
Copy link
Contributor Author

bawr commented Jun 3, 2016

Closing the file before we attempt to unlink it -- if there is an error.

Code talks better than descriptions, so I guess later today I'll just revert this, make that tiny change, and you can take it from there. No point in spending more time than that, when the fix I have in mind is literally on the order of four lines. ;)

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