classification
Title: Allow passing "delete=False" to TemporaryDirectory
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.6
process
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: Nosy List: Anthony Sottile, CAM-Gerlach, epicfaace, georg.brandl, maurosr, ncoghlan, nyuszika7h, pitrou, r.david.murray, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2015-09-08 05:36 by Antony.Lee, last changed 2021-03-08 23:04 by CAM-Gerlach. This issue is now closed.

Files
File name Uploaded Description Edit
TemporaryDirectory_delete_false.patch maurosr, 2015-10-06 04:39 review
Pull Requests
URL Status Linked Edit
PR 24793 merged CAM-Gerlach, 2021-03-08 23:04
Messages (20)
msg250158 - (view) Author: Antony Lee (Antony.Lee) * Date: 2015-09-08 05:36
I would like to suggest allowing passing "delete=False" to the TemporaryDirectory constructor, with the effect that the directory is not deleted when the TemporaryDirectory context manager exits, or when the TemporaryDirectory object is deleted.

I realize that this would effectively duplicate the functionality of mkdtemp, but this is similar to the redundancy between NamedTemporaryFile(delete=False) and mkstemp, and would make it easier to switch between the two behaviors (which is useful e.g. when debugging, where you may need to look at the temporary files "post-mortem").
msg250217 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-09-08 15:31
The two cases are not parallel, in that NamedTemporaryFile closes the file at the end of the context manager, whereas the *only* thing the TemporaryDirectory does is delete the directory on cleanup.

There is some value to the "parallel interface" argument, so I'm only -0 on this.
msg250386 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-09-10 16:04
I'm inclined to reject this idea too.
msg250387 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2015-09-10 16:13
The idea looks ok to me, but you'd have to provide a patch.
msg252375 - (view) Author: Mauro S. M. Rodrigues (maurosr) * Date: 2015-10-06 04:39
Hi everybody!
This is my second patch on the community, although the first one is not merged, so any feedback is appreciated. 

I've added tests to cover this new situation and docs to let people know about the possibility of keeping their temporary directories for debugging purposes. 

Regarding the code itself, I've also made a 'design decision' to remove the directory even when created with delete=False if cleanup method is called explicitly, so I would like to hear your thoughts on that matter specially if you don't agree with it.
msg364345 - (view) Author: Anthony Sottile (Anthony Sottile) * Date: 2020-03-16 17:46
I would certainly like to see this, it would eliminate my last few hand rolled temporary directory contexts

Mauro would you be interested in re-posting this patch as a PR to github? (or allowing someone else to carry your patch forward?)
msg364453 - (view) Author: Mauro S. M. Rodrigues (maurosr) * Date: 2020-03-17 17:04
Hi Anthony, 

Thanks for asking, yeah I'm interested in push a new version. I'll do it later today and I'll post a link to the pr here.
msg364458 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-03-17 17:26
What is your use case Anthony?
msg364461 - (view) Author: Anthony Sottile (Anthony Sottile) * Date: 2020-03-17 17:37
one example is here: https://github.com/pre-commit/pre-commit/blob/bb6f1efe63c168d9393d520bd60e16c991a57059/pre_commit/store.py#L137-L139

where I would want cleanup in the exceptional case

another (related but different) closed-source use case involves creating a temporary directory that may be deleted and currently has to be guarded by:

    shutil.rmtree(..., ignore_errors=True)

(making `TemporaryDirectory` not useful since it crashes if the directory goes missing)

there's additionally the problem of readonly files on windows, and readonly directories elsewhere being undeletable without a custom rmtree but I think that's a different issue entirely -- https://github.com/pre-commit/pre-commit/blob/bb6f1efe63c168d9393d520bd60e16c991a57059/pre_commit/util.py#L250-L267
msg364466 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-03-17 17:56
If you want the conditional cleanup, then TemporaryDirectory is obviously a wrong tool. mkdtemp looks more appropriate.

If you remove directory in process of working with temporary directory, would it help if you create a subdirectory in the temporary directory and work with it (including conditionally removing)?

As for the readonly files, was not this problem solved by PR 10320?
msg364468 - (view) Author: Anthony Sottile (Anthony Sottile) * Date: 2020-03-17 18:06
oh!  that's neat, yeah hadn't been following closely enough it seems, good to hear that the readonly thing is fixed!
msg365103 - (view) Author: Mauro S. M. Rodrigues (maurosr) * Date: 2020-03-26 17:49
So per Serhiy comment can I assume the patch is not necessary? If so I believe the issue should be closed as well.
msg365105 - (view) Author: Anthony Sottile (Anthony Sottile) * Date: 2020-03-26 17:59
Serhiy's comment provides workarounds but are still (imo) inferior to supporting this.

I would really like for this api to match that of NamedTemporaryFile which has the same `delete=...` option
msg365106 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-03-26 18:16
Could you please explain the difference between hypothetical TemporaryDirectory(delete=False) and simple mkdtemp()?
msg365107 - (view) Author: Anthony Sottile (Anthony Sottile) * Date: 2020-03-26 18:25
the differences are api compatibility with context manager protocol and equivalence with NamedTemporaryFile

NamedTemporaryFile(delete=False) is just `mkstemp` afaict and the api is there
msg365108 - (view) Author: Anthony Sottile (Anthony Sottile) * Date: 2020-03-26 18:26
you are right though, the effect is the same as just using mkdtemp
msg365109 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-03-26 19:32
Well, then I think there is nothing to do this.

Compatibility with context manager protocol is not a goal if it does nothing on exit. In any case you can easy to make it in three lines:

@contextmanager
def mymkdtemp():
    yield mkdtemp()

NamedTemporaryFile is more complex than mkstemp(). It creates a file object and closes a file descriptor even if leave the file on the filesystem.
msg382595 - (view) Author: Richard (nyuszika7h) Date: 2020-12-06 14:10
Sorry for reviving a 9 months old issue, but IMO there was no good reason to reject this especially when a patch was provided. Even if the context manager can be replaced with 3 lines of code, I still don't consider that very user-friendly.

My use case would be passing `delete=False` temporarily while debugging my script, it would be much simpler than using a whole different hacky method when the end goal is to change it back to `delete=True` once it is completed anyway.

What issues exactly does the addition of a simple option cause? I don't think something as trivial as this causes a maintenance burden, and you can't call it feature creep either when TemporaryDirectory doesn't have *any* other optional keyword arguments.
msg387785 - (view) Author: Ashwin Ramaswami (epicfaace) * Date: 2021-02-27 19:33
I agree -- as a user, it wasn't clear to me from looking at the documentation that mkdtemp was the right way to go to not delete directories. I had expected that NamedTemporaryDirectory would also support delete=False, just like NamedTemporaryFile.


Given that we say in the documentation that "mkstemp() and mkdtemp() are lower-level functions which require manual cleanup", it seems to make sense to allow Python users who don't care / know about mkstemp / mkdtemp to just use the more user-friendly NamedTemporaryDirectory / NamedTemporaryFile classes. This would make the language more accessible.

Would we be fine with reopening this issue so someone can contribute a patch?
msg388220 - (view) Author: C.A.M. Gerlach (CAM-Gerlach) * Date: 2021-03-07 03:14
To note, the proposal on [BPO-29982](https://bugs.python.org/issue29982) should hopefully address one of the two use-cases described by Anthony Sotittle, `ignore_errors=True`, in a cleaner fashion that takes advantage of the existing higher-level functionality rather than duplicating `mkdtemp`. Opinions and feedback welcome over there.
History
Date User Action Args
2021-03-08 23:04:18CAM-Gerlachsetpull_requests: + pull_request23561
2021-03-07 03:14:34CAM-Gerlachsetnosy: + CAM-Gerlach
messages: + msg388220
2021-02-27 19:33:33epicfaacesetnosy: + epicfaace
messages: + msg387785
2020-12-06 14:10:13nyuszika7hsetnosy: + nyuszika7h
messages: + msg382595
2020-03-26 19:32:11serhiy.storchakasetstatus: open -> closed
resolution: rejected
messages: + msg365109

stage: resolved
2020-03-26 18:26:24Anthony Sottilesetmessages: + msg365108
2020-03-26 18:25:01Anthony Sottilesetmessages: + msg365107
2020-03-26 18:16:33serhiy.storchakasetmessages: + msg365106
2020-03-26 17:59:10Anthony Sottilesetmessages: + msg365105
2020-03-26 17:49:55maurosrsetmessages: + msg365103
2020-03-17 18:06:54Anthony Sottilesetmessages: + msg364468
2020-03-17 17:56:02serhiy.storchakasetmessages: + msg364466
2020-03-17 17:37:56Anthony Sottilesetmessages: + msg364461
2020-03-17 17:26:00serhiy.storchakasetmessages: + msg364458
2020-03-17 17:06:06Antony.Leesetnosy: - Antony.Lee
2020-03-17 17:04:51maurosrsetmessages: + msg364453
2020-03-16 17:46:54Anthony Sottilesetnosy: + Anthony Sottile
messages: + msg364345
2015-10-06 04:39:23maurosrsetfiles: + TemporaryDirectory_delete_false.patch

nosy: + maurosr
messages: + msg252375

keywords: + patch
2015-09-10 16:13:54pitrousetmessages: + msg250387
2015-09-10 16:04:24serhiy.storchakasetnosy: + georg.brandl, ncoghlan, pitrou, serhiy.storchaka
messages: + msg250386
2015-09-08 15:31:48r.david.murraysettype: enhancement

messages: + msg250217
nosy: + r.david.murray
2015-09-08 05:36:20Antony.Leecreate