Skip to content

Close file handle before passing edit's tempfile to editor.#416

Closed
minchinweb wants to merge 1 commit intojrnl-org:masterfrom
minchinweb:2.0-tmpfile-fix
Closed

Close file handle before passing edit's tempfile to editor.#416
minchinweb wants to merge 1 commit intojrnl-org:masterfrom
minchinweb:2.0-tmpfile-fix

Conversation

@minchinweb
Copy link
Contributor

Fix for #415 on Windows.

@xemoka
Copy link

xemoka commented Jan 31, 2017

Will this ever get merged or is there some conflict for doing it this way?

notbalanced added a commit to notbalanced/jrnl that referenced this pull request Mar 1, 2017
minchinweb added a commit to minchinweb/jrnl that referenced this pull request Nov 14, 2017
@mrlo
Copy link

mrlo commented Aug 15, 2018

Also looking to see if this is going to get merged...

@arisAlexis
Copy link

bump

@wren wren changed the base branch from 2.0-rc1 to master July 7, 2019 01:20
@wren
Copy link
Member

wren commented Jul 7, 2019

This looks like a great fix, and we'd love to merge it. Would you mind resolving the merge conflict?

@wren wren added bug Something isn't working windows labels Jul 7, 2019
@minchinweb
Copy link
Contributor Author

@wren Will do!

@minchinweb minchinweb dismissed a stale review via 64d9b1b July 8, 2019 22:09
@minchinweb
Copy link
Contributor Author

@wren: the pull request has been recreated and passes the test suite. I'm don't think there was a test in place for this particular issue.

@minchinweb minchinweb mentioned this pull request Jul 8, 2019
@wren
Copy link
Member

wren commented Jul 9, 2019

@minchinweb It looks like this was maybe already fixed by another pr? Sorry for the back and forth, but I hadn't seen the other one when I commented here.

Is there some reason the os.close call needs to be moved up a few lines like this PR does?

@minchinweb
Copy link
Contributor Author

@wren: yes, this is almost identical to #564, except that that one got applied to the 1.9.x branch, but has never been merged into the 2.0 branch; this one applies directly to the 2.0 branch. So this should be applied to the 2.0 branch, or #564 should be cherry-picked to the 2.0 branch.

As for the location of closing the filehandle, first here is the code:

    filehandle, tmpfile = tempfile.mkstemp(prefix="jrnl", text=True, suffix=".txt")
    os.close(filehandle)  # in PR 564
    with codecs.open(tmpfile, 'w', "utf-8") as f:
        if template:
            f.write(template)
    os.close(filehandle)  # in PR 416 (this one)

Personally, I like it lower, as it maintains the file lock if we decide to write the template file to our temp file.

@stale
Copy link

stale bot commented Sep 17, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Inactive issue: will be closed soon if no activity label Sep 17, 2019
@minchinweb minchinweb removed the stale Inactive issue: will be closed soon if no activity label Sep 17, 2019
@sergiorgiraldo
Copy link

Hi, it seems this continues in version 1.9.8. Can this be merged?

@wren
Copy link
Member

wren commented Sep 25, 2019

@sergiorgiraldo v2.0 is out now, and I believe has a fix for this already. Can you please upgrade from v1.9.8 and confirm the fix?

@wren
Copy link
Member

wren commented Sep 25, 2019

Already fixed.

@wren wren closed this Sep 25, 2019
@minchinweb minchinweb deleted the 2.0-tmpfile-fix branch December 14, 2019 21:12
@wren
Copy link
Member

wren commented Jan 10, 2020

@sergiorgiraldo Sorry, this wasn't actually fixed, apparently! It looks like @micahellison has submitted the actual fix here.

@lock
Copy link

lock bot commented May 21, 2020

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot added the 🔒 Outdated label May 21, 2020
@lock lock bot locked as resolved and limited conversation to collaborators May 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bug Something isn't working 🔒 Outdated windows

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants