tempfile docs can use some refreshing.

Thanks for the patch, Zbyszek. For future reference, please send patches from the Mercurial repository. It will make the patch review process easier. See also devguide.

+:func:`TemporaryDirectory`, :func:`SpooledTemporaryFile`, which can be

Both TemporaryDirectory and SpooledTemporaryFile are actually a class. See https://hg.python.org/cpython/file/3.4/Lib/tempfile.py#l510

+ :ref:`context-managers`). On completion of the context or

:ref:`context-managers` link is unnecessary in my opinion. It would be better to link https://docs.python.org/3/library/tempfile.html#examples

(Please update tempfile.TemporaryDirectory docs too.)

It would also be helpful to have a patch with minimum changes (that is, do not reflow the lines).

v2:

• remove reflows
• update TemporaryDirectory description too
• do not call things which are not functions "functions"
• with O_TMPFILE the file is not unlinked, also update TemporaryFile description for that
• link to Examples

[Padding]

Please start sentences with capital letters, specifically “mkstemp() and mkdtemp() are lower-level functions . . .”.

The new sentence at the top about context managers seems out-of-place. Perhaps something like “They can also be used as context managers, which triggers the cleanup on exit.”

The new paragraph about cleanup of TemporaryFile is good, but I think it now makes the last sentence of that entry redundant: “. . . can be used in a ‘with’ statement . . .”

If the first word is a function name that does not start with a capital, the sentence can't start with a capital. Sometimes it is worthwhile to reword the sentence so you can start it with a capitalizable word, but sometimes it isn't. (I haven't reviewed the patch, just putting in my two cents on this particular topic ;)

Please start sentences with capital letters, specifically “mkstemp() and mkdtemp() are lower-level functions . . .”.

This would make the sentence more convoluted... I think that with markup it is pretty clear that this is a function name and the lowercase letter it not misleading.

The new sentence at the top about context managers seems out-of-place. Perhaps something like “They can also be used as context managers, which triggers the cleanup on exit.”

The two sentences are merged now.

The new paragraph about cleanup of TemporaryFile is good, but I think it now makes the last sentence of that entry redundant: “. . . can be used in a ‘with’ statement . . .”

Yes, removed.

v3:

• remove sentence “. . . can be used in a ‘with’ statement . . .”
• merge two senteces in first paragraph

The current proposed text would be rendered something like this:

'''
TemporaryFile, NamedTemporaryFile, TemporaryDirectory, SpooledTemporaryFile are high-level interfaces which provide automatic cleanup and can be used as context managers. mkstemp() and mkdtemp() are lower-level functions which require manual cleanup.
'''

My opinion is it is very often worthwhile rewording the sentence. It is awkward to read at the moment, especially if you are trying to skim over it without reading the whole paragraph. Unlike in Python, in English there’s always more than one way to do it. Perhaps you could consider this version instead:

'''
The TemporaryFile, NamedTemporaryFile, TemporaryDirectory, [and] SpooledTemporaryFile classes are high-level interfaces which provide automatic cleanup and can be used as context managers. The mkstemp() and mkdtemp() functions are lower-level, and require manual cleanup.
'''

Only about three more words, and not particularly convoluted. But if you disagree, I can live with it, since there are plenty more examples of this in the Python documentation and elsewhere :)

Other than that, I think the patch is good.

Actually they are not classes, so the proposed wording cannot be used. But indeed it sounds better with the "and".

v4:

• one more "and"

Ping?

I'm re-uploading the patch as an hg diff so that it gets a review link.

Thanks for reformatting the patch. I made some review comments.

Replying to review here... I get 500 server error in the patch review reply dialogue :(

On 2015/04/15 02:40:14, r.david.murray wrote:

http://bugs.python.org/review/23725/diff/14592/Doc/library/tempfile.rst
File Doc/library/tempfile.rst (left):

http://bugs.python.org/review/23725/diff/14592/Doc/library/tempfile.rst#oldcode55
Doc/library/tempfile.rst:55: :keyword:`with` statement, just like a normal file.
Why did you remove this statement?
It is redundant. The fact that this can be used as CM is already mentioned in the introduction and in the description of TemporaryFile.

http://bugs.python.org/review/23725/diff/14592/Doc/library/tempfile.rst
File Doc/library/tempfile.rst (right):

http://bugs.python.org/review/23725/diff/14592/Doc/library/tempfile.rst#newcode25
Doc/library/tempfile.rst:25: The need to use the insecure :func:`mktemp`
function is eliminated.
How about we get even more radical. Let's eliminate the mention of mktemp from
the documentation, except for a "Deprecated Functions" section at the end, where
we explain that it is deprecated because it is insecure and anything you could
do with it you can do with the un-deprecated functions.
Agreed. I'll submit a new version which removes all the historical notes and adds a "Deprecated" section at the end for tempdir and mktemp.

http://bugs.python.org/review/23725/diff/14592/Doc/library/tempfile.rst#newcode27
Doc/library/tempfile.rst:27: instead a string of six random characters is used.
Let's likewise eliminate the mention of the process id, and just leave the
explanation that six random characters are used.
OK.

http://bugs.python.org/review/23725/diff/14592/Doc/library/tempfile.rst#newcode31
Doc/library/tempfile.rst:31: directories. It is no longer necessary to use the
global *tempdir* variable.
The global tempdir variable can likewise be moved to the deprecated section and
removed from mention here.
OK.

http://bugs.python.org/review/23725/diff/14592/Doc/library/tempfile.rst#newcode42
Doc/library/tempfile.rst:42: collected). Under Unix, the directory entry for
the file is either not created at all or removed
"or is removed"
OK.

http://bugs.python.org/review/23725/diff/14592/Doc/library/tempfile.rst#newcode247
Doc/library/tempfile.rst:247:
There should be another blank line here.

v5:

• relegate tempdir and mktemp descriptions to 'Deprecated functions and variables' section at the end. This requires moving some descriptions around.
• add missing "is" pointed out in review
• add missing 's'

Made one minor suggestion in review comments (related to that deleted line). Otherwise this looks good to me, thanks.

Oh, because of the O_TMPDIR bits, this patch is only applicable to 3.5, so I'm removing 3.4 from versions. I don't think it is worth it to make a version that would apply to 3.4, since it is not the case that the 3.4 docs are *wrong*.

v6:

• add newline

I'm not 100% sure that tempfile.tempdir should be deprecated. Its much less convenient to control global behaviour with that. I agree that mktemp should be.

I've updated the patch though.

Well, something is wrong, because if we are deprecating :data:`tmpdir` we shouldn't be cross referencing it from the non-deprecated docs. The explanation of how the default value is calculated should be moved up to gettmpdir.

My understanding is that now that all functions accept a 'dir' parameter, the tmpdir global was deprecated because global state is bad, and if an application really wants to affect the global state it can use the one of the environment variables that are checked, which is more flexible.

I can't find any reference to a discussion to deprecate tempdir outside of this issue. Nothing on python-dev/python-ideas/peps.

I can see that there's an argument that it should be deprecated, but AFAICT the idea to do so originated here. I'd like to see wider discussion though (because I don't think it makes sense to deprecate it) - its no more or less global than environment variables, its already here as a feature, and its not going to save us code maintenance or bugs if we do deprecate it - and if we are deprecating it, we should be issueing a PendingDeprecation warning at minimum.

How about we change the patch to not deprecate it, and either open a new ticket, or let folk that want to deprecate it raise that on the list?

I don't think tempdir should be removed. I just think it should not be used. So what about moving the description of tempdir to the end, as it was in the last patch, but calling the section "Other functions and variables".

mktemp is clearly insecure. I'd just move the tmpdir up above the Deprecation section start

Updated version (based on issue-23725.patch from rbcollins):

• move tempdir description at the end of the main section, before Examples
• do not add my name second time in ACKS

Heh. This was really bugging me. I remembered it clearly, but I couldn't find an issue. Thought maybe it was a comment in the code, but nope. Google led me to this line in the docs: "It is no longer necessary to use the global tempdir variable." Checking when that was last changed, it turns out it was by me in bpo-10354, which is the issue containing the discussion that I was remembering, but that was about template.

Nevertheless, I think the sentence in the docs itself could be considered a documentation deprecation.

Since you are arguing that it shouldn't be considered deprecated, I think we should get some more participants in the discussion. I'm +.5 on explicitly documenting it as deprecated. I'm adding a couple folks from bpo-10354 who might have an opinion.

Note that if we do keep it in the main section, that sentence should still be deleted, since it looks like "it is no longer necessary" has been true now for longer than it *was* necessary.

Sorry, I didn't realise that Zbigniew was an alternative spelling of your first name.

New changeset 51d00482d403 by Robert Collins in branch '3.5':
Issue bpo-23725: Overhaul tempfile docs.
https://hg.python.org/cpython/rev/51d00482d403

New changeset 256d2f01e975 by Robert Collins in branch 'default':
Issue bpo-23725: Overhaul tempfile docs.
https://hg.python.org/cpython/rev/256d2f01e975

Thanks for the patch. I've committed the current status as an unambiguous improvement; we can add tempdir as deprecated later if there is consensus on that, the current patch did improve its docs per R. David Murray's request anyhow.