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

Allow delete=False for tempfile.TemporaryDirectory() #100131

Closed
JakobDev opened this issue Dec 9, 2022 · 12 comments
Closed

Allow delete=False for tempfile.TemporaryDirectory() #100131

JakobDev opened this issue Dec 9, 2022 · 12 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@JakobDev
Copy link
Contributor

JakobDev commented Dec 9, 2022

Feature or enhancement

If you use tempfile.TemporaryDirectory(), the directory is automagically deleted, what is the whole purpose of this class. But sometimes you may want to take a look at it for debugging reasons, so it would be nice to have a delete=False to prevent deleting.

Pitch

Code using this could look like this:

import argparse
import tempfile


def main():
    parser = argparse.ArgumentParser()
    parser.add_argument("--keep-temp-files", action="store_true", help="Don't delete temporary files")
    args = parser.parse_args()

    with tempfile.TemporaryDirectory(delete=not args.keep_temp_files) as tempdir:
        # Do something

        if args.keep_temp_files:
            print(f"You can view your temporary files at {tempdir}")


if __name__ == "__main__":
    main()

I personal use tempfile.TemporaryDirectory() a lot, especial in build scripts. Sometimes a need to take a look at the content of these. so having this Option would be great for that.

Other ways to achieve this

Of course, there would be other ways to achieve this.

1. Use a extra function that takes the directory as argument:

if args.keep_temp_files:
    do_something("some/other/dir")
else:
    with tempfile.TemporaryDirectory() as tempdir:
        do_something(tempfile)

This would need some code rewrite.

2. Using input()

with tempfile.TemporaryDirectory() as tempdir:
    # Do something

    if args.keep_temp_files:
        print(f"You can view your temporary files at {tempdir}. Press enter to delete the files.")
        breakpoint()

This could cause problems in situations, where you can't easily give some Keyboard input to TTY.

3. Using a custom function

def my_temp_dir(delet: bool = false):
    if delete:
        return tempfile.TemporaryDirectory()
    else:
        return my_custom_handler()

This will blow up your code when you just write a simple script. Python also often used by beginners, which may don't know, they can do this and start refactoring their code.

Why you should implement this

The argument can be easy added to exiting scripts if needed, without any additional work. A scripting language like Python should allow this to make debug more easy and more beginner friendly.

It is absolutely no work to implement this. If this would be much work to implement and maintain this, I would not suggested it. But in this case, it's just 3 steeps:

  1. Take a additional argument in __init__():
  2. Save is as class attribute
  3. In __exit__() ask, if it should delete or not

Previous discussion

None

Linked PRs

@JakobDev JakobDev added the type-feature A feature request or enhancement label Dec 9, 2022
@AlexWaygood AlexWaygood added the stdlib Python modules in the Lib dir label Dec 9, 2022
@CAM-Gerlach
Copy link
Member

See #69212 for the previous issue on this. FWIW, I was +/- 0 at the time, as I'd solved one of the previously-stated use cases in a cleaner fashion in #74168 , but I've come around to this idea as there are several other use cases for it presented there.

@serhiy-storchaka
Copy link
Member

Just use mkdtemp().

The purpose of the context manager is automatic deletion of the directory when you leave the block. If you do not need this, you do not need a contect manager.

@graingert
Copy link
Contributor

graingert commented Mar 23, 2023

there's one additional way to archive this:

with (
    contextlib.nullcontext(tempfile.mkdtemp())
    if args.keep_temp_files
    else tempfile.TemporaryDirectory()
) as dir:
    do_something(dir)

@JakobDev
Copy link
Contributor Author

Just use mkdtemp().

The purpose of the context manager is automatic deletion of the directory when you leave the block. If you do not need this, you do not need a contect manager.

I know that. The tempfile.TemporaryDirectory() is used for exactly that purpose. But sometimes, you need to debug a code that uses the context manager. So this Feature Request just adds for a Way to disable the normal behaviour ( using a breakpoint is not always possible/practical) , so you you can do your debugging stuff without doing much code rewrite and after that, you just remove the delete=False. This is not meant to be used, when you permanently don't want to delete the temp directory.

there's one additional way to archive this:

This is not suitable, if you are not a Python expert

@gpshead
Copy link
Member

gpshead commented Mar 23, 2023

I'm going to close this based on Serhiy's practical mkdtemp() suggestion above. We've already got one way to do this.

@gpshead gpshead closed this as not planned Won't fix, can't repro, duplicate, stale Mar 23, 2023
@zware
Copy link
Member

zware commented Mar 23, 2023

The point of TemporaryDirectory is to make handling the lifecycle of a temporary directory convenient. There are other ways to achieve the desired result, but it's hard to call any of them convenient.

@CAM-Gerlach
Copy link
Member

FWIW, given the basic stated motivation for this feature, both on this issue and the previous one, is cases in which the cleanup may or may not be required, the decent number of users who presented similar use case, it's not clear to me how "just use mkdtemp" is a good solution for that.

Sure, there are workarounds, but the OP illustrates the issues with each, and in summary the obvious ones are kludgy and the non-kludgy ones are not obvious. It also seems like it could increase the chance users reach for the more cryptic lower-level interfaces for the other cases where the higher-level one would be safer, simpler and more ergonomic.

In light of the fact that the code changes to the stdlib are relatively minimal, and there's an up to date PR with tests and docs provided, I'm inclined to agree with @zware that this change shouldn't be rejected on that basis.

@gpshead
Copy link
Member

gpshead commented Mar 24, 2023

fair enough. I really don't think this feature will see much use, but I've used the PR to improve the docstring anyways.

@gpshead gpshead reopened this Mar 24, 2023
@gpshead gpshead self-assigned this Mar 24, 2023
@AlexWaygood
Copy link
Member

It still feels quite odd to me to say "We have a TemporaryDirectory class, but it's not always temporary -- you can make it not-temporary by using this special flag we added".

There's been a number of different ways of doing this pointed out in the above thread, but FWIW I'd probably use a contextlib.ExitStack to achieve this:

with ExitStack() as stack:
    if args.keep_temp_files:
        dir = stack.enter_context(TemporaryDirectory())
    else:
        dir = mkdtemp()
    do_something(dir)

@carljm
Copy link
Member

carljm commented Mar 24, 2023

It still feels quite odd to me to say "We have a TemporaryDirectory class, but it's not always temporary -- you can make it not-temporary by using this special flag we added".

I don't think arguing on the basis of the meaning of "temporary" holds much water here. mkdtemp() also has "temp" in the name and lives in tempfile.py, but that doesn't mean Python deletes it for you. "Temporary" here has a broader meaning: it's created in some distinguished location for throwaway files, where quite possibly some OS-level automated process may come along and clean it up at some point.

The ExitStack solution and the nullcontext solution are both just fine, but they are quite a bit more verbose. The use-case that I find most convincing is the "temporary (ahem) debugging" use case, where you have a TemporaryDirectory that you generally do want cleaned up, but in the course of some debugging you need to leave it around to look at. I've definitely had this situation, and reindenting a bunch of code or adding a new import for nullcontext are not very attractive quick options.

@gpshead
Copy link
Member

gpshead commented Mar 24, 2023

It may seem odd at first glance, but there are even non-debugging uses - you may have code that needs cleanup to be conditional based on user input. This, perhaps unusual, feature provides an easy way to do that without adopting unusual code structures.

I added the missing documentation to the PR and described this in there as I expect others to look at the feature with the same surprise I initially had as well.

gpshead added a commit that referenced this issue Mar 24, 2023
…ry() (#100132)

Add optional delete parameter to tempfile.TemporaryDirectory().

Co-authored-by: Gregory P. Smith <greg@krypto.org>
@gpshead
Copy link
Member

gpshead commented Mar 24, 2023

Thanks all, including the contributed PR and the discussion. Merged. You changed my mind. :)

@gpshead gpshead closed this as completed Mar 24, 2023
Fidget-Spinner pushed a commit to Fidget-Spinner/cpython that referenced this issue Mar 27, 2023
…irectory() (python#100132)

Add optional delete parameter to tempfile.TemporaryDirectory().

Co-authored-by: Gregory P. Smith <greg@krypto.org>
warsaw pushed a commit to warsaw/cpython that referenced this issue Apr 11, 2023
…irectory() (python#100132)

Add optional delete parameter to tempfile.TemporaryDirectory().

Co-authored-by: Gregory P. Smith <greg@krypto.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

8 participants