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

Add context manager for temporary directory #49428

Closed
nascheme opened this issue Feb 7, 2009 · 14 comments
Closed

Add context manager for temporary directory #49428

nascheme opened this issue Feb 7, 2009 · 14 comments
Assignees
Labels
type-feature A feature request or enhancement

Comments

@nascheme
Copy link
Member

nascheme commented Feb 7, 2009

BPO 5178
Nosy @nascheme, @birkenfeld, @ncoghlan, @pitrou, @giampaolo, @ezio-melotti
Files
  • tempdir.patch
  • tempdir2.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/ncoghlan'
    closed_at = <Date 2010-10-24.11:24:09.583>
    created_at = <Date 2009-02-07.17:54:19.428>
    labels = ['type-feature']
    title = 'Add context manager for temporary directory'
    updated_at = <Date 2011-09-20.10:32:01.495>
    user = 'https://github.com/nascheme'

    bugs.python.org fields:

    activity = <Date 2011-09-20.10:32:01.495>
    actor = 'daniel.ugra'
    assignee = 'ncoghlan'
    closed = True
    closed_date = <Date 2010-10-24.11:24:09.583>
    closer = 'ncoghlan'
    components = []
    creation = <Date 2009-02-07.17:54:19.428>
    creator = 'nascheme'
    dependencies = []
    files = ['12970', '13080']
    hgrepos = []
    issue_num = 5178
    keywords = ['patch']
    message_count = 14.0
    messages = ['81342', '81349', '81995', '82044', '82085', '82088', '84005', '97706', '104435', '119452', '119463', '119509', '119546', '119596']
    nosy_count = 7.0
    nosy_names = ['nascheme', 'georg.brandl', 'ncoghlan', 'pitrou', 'giampaolo.rodola', 'ezio.melotti', 'daniel.ugra']
    pr_nums = []
    priority = 'normal'
    resolution = 'accepted'
    stage = 'patch review'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue5178'
    versions = ['Python 3.2']

    @nascheme
    Copy link
    Member Author

    nascheme commented Feb 7, 2009

    I noticed that it would be nice to have a temporary directory context
    manager while trying to fix a broken unittest. The attached patch
    provides a pretty minimal implementation. There appears to be lots of
    unit tests that could use such a thing (just search for "rmtree").

    I'm not sure if TemporaryDirectory is the best name. Also, perhaps it
    should have a __del__ method although that's tricky business.

    @nascheme nascheme added the type-feature A feature request or enhancement label Feb 7, 2009
    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Feb 7, 2009

    A __del__ method is definitely desirable (tempfile._TemporaryFileWrapper
    gives an example of how to cache the relevant globals on the class
    object to avoid attempting calls to None during interpreter shutdown).

    The new examples are good, but may give the misleading impression that
    TemporaryFile can't be used as a context manager (it can, and the file
    will be closed at the end of the with statement). A second file example
    showing it being used as a context manager would probably be helpful.

    There is also at least one existing contextlib based temp_dir context
    managers that could be replaced given the addition of this (i.e. in
    test_cmd_line_script.py, "test_dir" could be changed to a class that
    inherits from tempfile.TemporaryDirectory and overrides __enter__ to
    invoke realname() on the directory name).

    @ncoghlan ncoghlan changed the title Add content manager for temporary directory Add context manager for temporary directory Feb 8, 2009
    @nascheme
    Copy link
    Member Author

    New version of the patch. Added a __del__ method that hopefully works
    reliably. Added NEWS and an example of TemporaryFile as a context
    manager. I did not change more tests to use TemporaryDirectory since I
    understand hit-and-run modernization of code is generally discouraged.
    That said, I think there are some tests that could have their
    reliability improved by using TemporaryDirectory (I started on this
    patch when I saw some build bot failures). I'll look into that later.

    @pitrou
    Copy link
    Member

    pitrou commented Feb 14, 2009

    If you look at the code for os.path.isdir, it uses other globals such as
    os.stat, so you might want to reinject them in your class as well...

    @nascheme
    Copy link
    Member Author

    Argh, it's like pulling a sweater thread. The stat.S_ISDIR function
    uses the S_IFDIR global so I would have write my own. This does not
    look like good maintainable code, IMO.

    Maybe there should be a special flag modules can set to make them get
    cleared later in the interpreter cleanup procedure. Modules could set
    it if they don't create reference cycles themselves and don't hold
    references to user objects.

    @pitrou
    Copy link
    Member

    pitrou commented Feb 14, 2009

    Maybe there should be a special flag modules can set to make them get
    cleared later in the interpreter cleanup procedure. Modules could set
    it if they don't create reference cycles themselves and don't hold
    references to user objects.

    Or, perhaps we could remove the assignment to None and use a
    finalization scheme based on the garbage collector instead.
    See bpo-812369 (no patch for the above idea unfortunately).

    @ncoghlan
    Copy link
    Contributor

    Unassigning for the moment - I'm still interested in the idea, but the
    referencing-globals-at-shutdown problem means it isn't going to be the
    straightforward review-and-commit that I original thought this patch was
    going to be.

    I'll still try to get to it before the next 3.1 alpha in a couple of
    weeks, but I don't want to stop anyone else from looking at it in the
    meantime.

    @ncoghlan ncoghlan removed their assignment Mar 23, 2009
    @ncoghlan
    Copy link
    Contributor

    Building a collection of issues I want to take a look at for 2.7

    @ncoghlan ncoghlan self-assigned this Jan 13, 2010
    @ncoghlan
    Copy link
    Contributor

    Missed the boat for 2.7 I'm afraid. Definitely one to take another look at for 3.2 though.

    @ncoghlan ncoghlan removed their assignment Apr 28, 2010
    @birkenfeld
    Copy link
    Member

    Ping... soon it's too late for 3.2 too :)

    @giampaolo
    Copy link
    Contributor

    Personally I would like to have a unique tempfile.mkdtemp which can be used as both a function and a context manager, as it happens for open() and others.
    Not sure how to do that though, unless tempfile.mkdtemp gets turned into a class.
    There would be objections against doing that?

    @ncoghlan ncoghlan self-assigned this Oct 24, 2010
    @ncoghlan
    Copy link
    Contributor

    Committed (with enhanced tests and a few fixes) in r85818

    And credit where it's due to test___all__ for picking up a typo in my adjustment to tempfile.__all__ :)

    I created issue bpo-10188 to track the shutdown problem. I'm considering taking out the code that attempts to allow the __del__ method to work at shutdown though, since it isn't actually achieving anything (I wanted a record of it in the main repository, which is why I kept it for the initial checkin).

    @giampaolo
    Copy link
    Contributor

    Nick, could you comment about my last proposal?

    @ncoghlan
    Copy link
    Contributor

    Merging the interfaces for mkdtemp and TemporaryDirectory isn't going to happen.

    mkstemp/mkdtemp are for when the user wants to control the lifecycle of the filesystem entries themselves. (Note that context management on a regular file-like object only closes the file, it doesn't delete it from the filesystem). They're also intended as relatively thin wrappers around the corresponding C standard library functionality.

    The other objects in tempfile (TemporaryFile, TemporaryDirectory, etc) are for when the user wants the lifecycle of the Python object to correspond with the lifecycle of the underlying filesystem element.

    That said, TD itself can be used to create the temporary directory without having to use it as a context manager (the underlying directory is created in __init__, not __enter__).

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants