Opened 5 years ago

Closed 4 years ago

#6165 enhancement closed fixed (fixed)

Docstring for twisted.trial.unittest.SynchronousTestCase.mktemp not informative enough

Reported by: Peter Stringfield Owned by: Tom Prince
Priority: low Milestone:
Component: trial Keywords: documentation
Cc: Jonathan Lange, Tom Prince Branch: branches/mktemp-api-docs-6165
branch-diff, diff-cov, branch-cov, buildbot
Author: exarkun

Description

twisted.trial.unittest.SynchronousTestCase.mktemp is used to create temporary directories and files in unit tests. This is its docstring:

        """
        Returns a unique name that may be used as either a temporary directory
        or filename.

        @note: you must call os.mkdir on the value returned from this method if
            you wish to use it as a directory!

        @return: C{str}
        """ 

Unique in what context? Could it conflict with a name outside the current working directory? Outside of a standard Twisted distribution? Or is it a magic function that magically gives universally unique names?

Generated Doc.s: http://twistedmatrix.com/documents/current/api/twisted.trial.unittest.SynchronousTestCase.html#mktemp

Source: http://twistedmatrix.com/trac/browser/tags/releases/twisted-12.2.0/twisted/trial/unittest.py#L947

Improved docstring:

        """
        Return a relative path that is guaranteed to be unique within the
        current working directory. Create every directory between the current 
        working directory and the last one if necessary. Do not create the 
        last directory/file.
        
        For a temporary directory call os.mkdir on the path, for a temporary
        file, just create the file (e.g. by opening the path for writing and
        then closing it).

        Trial should delete the temporary file automatically.

        @return: C{str}
        """

Internally it uses tempfile.mkdtemp (from the standard library), in combination with the name of the test module, test suite, and test.

It CREATES a short directory hierarchy like ".\twisted.test.test_compat\ExecfileCompatTestCase\test_execfileGlobals\fqsdxf" and returns a string like ".\twisted.test.test_compat\ExecfileCompatTestCase\test_execfileGlobals\fqsdxf\temp". The random portion is coming from tempfile.mkdtemp.

Attachments (3)

ticket_6165.diff (1.3 KB) - added by Peter Stringfield 5 years ago.
Improve docstring for twisted.trial._synctest.SynchronousTestCase.mktemp.
ticket_6165.2.diff (1.3 KB) - added by Peter Stringfield 5 years ago.
Fixed a dumb typo in the first patch's topfile entry.
ticket_6165_3.diff (1.3 KB) - added by Peter Stringfield 4 years ago.
Discovered misconfigured vimrc: was not auto-deleting trailing whitespace;; fixed whitespace.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 5 years ago by DefaultCC Plugin

Cc: Jonathan Lange added

Changed 5 years ago by Peter Stringfield

Attachment: ticket_6165.diff added

Improve docstring for twisted.trial._synctest.SynchronousTestCase.mktemp.

Changed 5 years ago by Peter Stringfield

Attachment: ticket_6165.2.diff added

Fixed a dumb typo in the first patch's topfile entry.

comment:2 Changed 5 years ago by Peter Stringfield

Keywords: review added

Changed 4 years ago by Peter Stringfield

Attachment: ticket_6165_3.diff added

Discovered misconfigured vimrc: was not auto-deleting trailing whitespace;; fixed whitespace.

comment:3 Changed 4 years ago by Tom Prince

Cc: Tom Prince added

Experimentally, it appears that trial doesn't in fact delete the directory so created.

comment:4 Changed 4 years ago by Peter Stringfield

You are correct. (Though Trial will delete the files the next time it runs.) Also the directory isn't even created in the CWD. I don't know where I got that. It's created in twisted/_trial_temp.

Maybe the original docstring is best afterall. Just call open or mkdir on the path and run with it, don't worry about where it is or what happens to it. Probably its not worth documenting anything more specific than that.

I'm inclined to close this as wontfix and forget about it. Sorry for the trouble.

comment:5 Changed 4 years ago by Tom Prince

Well, it is probably worth documenting that the path is relative to the current directory. And that the path returned is a fresh name in a directory (which is a subdirectory of the current directory).

comment:6 Changed 4 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Jean-Paul Calderone

Indeed. Thanks for working on this. It seems like at least part of this change is desirable. I'll see if I can split that part out.

comment:7 Changed 4 years ago by Jean-Paul Calderone

Author: exarkun
Branch: branches/mktemp-api-docs-6165

(In [36876]) Branching to 'mktemp-api-docs-6165'

comment:8 Changed 4 years ago by Jean-Paul Calderone

Keywords: documentation review added
Owner: Jean-Paul Calderone deleted

How about that.

comment:9 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Tom Prince

This looks good. I'll merge it.

comment:10 Changed 4 years ago by Tom Prince

Resolution: fixed
Status: newclosed

(In [37071]) Merge mktemp-api-docs-6165: Improve documentation of twisted.trial.unittest.SynchronousTestCase.mktemp

Author: meissenPlate, exarkun Reviewers: exarkun, tom.prince Fixes: #6165

The original documentation didn't give any direction no how it should be use used. Explain what it returns and how to use it.

Note: See TracTickets for help on using tickets.