classification
Title: update tempfile docs to say that TemporaryFile is secure
Type: enhancement Stage: resolved
Components: Documentation Versions: Python 3.6, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: docs@python Nosy List: berker.peksag, docs@python, eric.smith, gregory.p.smith, martin.panter, python-dev, r.david.murray, rbcollins, rhettinger, zbysz
Priority: normal Keywords: patch

Created on 2015-03-20 19:14 by zbysz, last changed 2015-08-12 23:41 by rbcollins. This issue is now closed.

Files
File name Uploaded Description Edit
0001-docs-update-description-of-TemporaryFile-and-tempfil.patch zbysz, 2015-03-20 19:14
0001-docs-update-description-of-TemporaryFile-and-tempfil.patch zbysz, 2015-03-22 16:00
0001-docs-update-description-of-TemporaryFile-and-tempfil.patch zbysz, 2015-03-23 02:16
0001-docs-update-description-of-TemporaryFile-and-tempfil.patch zbysz, 2015-03-23 05:03
tempfile_docs.patch r.david.murray, 2015-04-15 00:17 review
tempfile_docs.patch zbysz, 2015-04-18 15:23 v5 of patch review
tempfile_docs.patch zbysz, 2015-04-19 00:43 review
issue-23725.patch rbcollins, 2015-08-03 21:11
issue-23725.patch zbysz, 2015-08-04 00:48 v8 of the patch review
Messages (26)
msg238713 - (view) Author: Zbyszek Jędrzejewski-Szmek (zbysz) * Date: 2015-03-20 19:14
tempfile docs can use some refreshing.
msg238750 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2015-03-21 02:34
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.)
msg238759 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-03-21 04:19
It would also be helpful to have a patch with minimum changes (that is, do not reflow the lines).
msg238916 - (view) Author: Zbyszek Jędrzejewski-Szmek (zbysz) * Date: 2015-03-22 16:00
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
msg238959 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-03-22 23:57
[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 . . .”
msg238970 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-03-23 01:55
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 ;)
msg238972 - (view) Author: Zbyszek Jędrzejewski-Szmek (zbysz) * Date: 2015-03-23 02:16
> 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
msg238974 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-03-23 02:52
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.
msg238980 - (view) Author: Zbyszek Jędrzejewski-Szmek (zbysz) * Date: 2015-03-23 05:03
Actually they are not classes, so the proposed wording cannot be used. But indeed it sounds better with the "and".

v4:
- one more "and"
msg241060 - (view) Author: Zbyszek Jędrzejewski-Szmek (zbysz) * Date: 2015-04-15 00:03
Ping?
msg241062 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-04-15 00:17
I'm re-uploading the patch as an hg diff so that it gets a review link.
msg241063 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-04-15 00:40
Thanks for reformatting the patch.  I made some review comments.
msg241420 - (view) Author: Zbyszek Jędrzejewski-Szmek (zbysz) * Date: 2015-04-18 15:19
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'
msg241447 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-04-18 19:14
Made one minor suggestion in review comments (related to that deleted line).  Otherwise this looks good to me, thanks.
msg241448 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-04-18 19:16
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*.
msg241467 - (view) Author: Zbyszek Jędrzejewski-Szmek (zbysz) * Date: 2015-04-19 00:43
v6:
- add newline
msg247954 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-08-03 21:11
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.
msg247958 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-08-03 22:11
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.
msg247959 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-08-03 22:17
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?
msg247962 - (view) Author: Zbyszek Jędrzejewski-Szmek (zbysz) * Date: 2015-08-03 22:35
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".
msg247968 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-08-04 00:20
mktemp is clearly insecure. I'd just move the tmpdir up above the Deprecation section start
msg247970 - (view) Author: Zbyszek Jędrzejewski-Szmek (zbysz) * Date: 2015-08-04 00:48
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
msg247974 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-08-04 02:30
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 issue 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 issue 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.
msg248482 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-08-12 22:37
Sorry, I didn't realise that Zbigniew was an alternative spelling of your first name.
msg248489 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-08-12 23:38
New changeset 51d00482d403 by Robert Collins in branch '3.5':
Issue #23725: Overhaul tempfile docs.
https://hg.python.org/cpython/rev/51d00482d403

New changeset 256d2f01e975 by Robert Collins in branch 'default':
Issue #23725: Overhaul tempfile docs.
https://hg.python.org/cpython/rev/256d2f01e975
msg248490 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-08-12 23:41
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.
History
Date User Action Args
2015-08-12 23:41:21rbcollinssetstatus: open -> closed
resolution: fixed
messages: + msg248490

stage: commit review -> resolved
2015-08-12 23:38:58python-devsetnosy: + python-dev
messages: + msg248489
2015-08-12 22:37:10rbcollinssetmessages: + msg248482
2015-08-04 02:30:10r.david.murraysetnosy: + rhettinger, gregory.p.smith, eric.smith
messages: + msg247974
2015-08-04 00:48:21zbyszsetfiles: + issue-23725.patch

messages: + msg247970
2015-08-04 00:20:26rbcollinssetmessages: + msg247968
2015-08-03 22:35:22zbyszsetmessages: + msg247962
2015-08-03 22:17:44rbcollinssetmessages: + msg247959
2015-08-03 22:11:54r.david.murraysetmessages: + msg247958
2015-08-03 21:11:07rbcollinssetfiles: + issue-23725.patch
nosy: + rbcollins
messages: + msg247954

2015-08-03 21:00:31rbcollinssetversions: + Python 3.6
2015-04-19 17:54:23r.david.murraysetstage: patch review -> commit review
2015-04-19 00:44:00zbyszsetfiles: + tempfile_docs.patch

messages: + msg241467
2015-04-18 19:16:31r.david.murraysetmessages: + msg241448
versions: - Python 3.4
2015-04-18 19:14:36r.david.murraysetmessages: + msg241447
2015-04-18 15:23:05zbyszsetfiles: + tempfile_docs.patch
2015-04-18 15:19:26zbyszsetmessages: + msg241420
2015-04-15 00:40:57r.david.murraysetmessages: + msg241063
2015-04-15 00:17:35r.david.murraysetfiles: + tempfile_docs.patch

messages: + msg241062
2015-04-15 00:03:51zbyszsetmessages: + msg241060
2015-03-23 05:03:31zbyszsetfiles: + 0001-docs-update-description-of-TemporaryFile-and-tempfil.patch

messages: + msg238980
2015-03-23 02:52:45martin.pantersetmessages: + msg238974
2015-03-23 02:16:13zbyszsetfiles: + 0001-docs-update-description-of-TemporaryFile-and-tempfil.patch

messages: + msg238972
2015-03-23 01:55:45r.david.murraysetmessages: + msg238970
2015-03-22 23:57:49martin.pantersetnosy: + martin.panter
messages: + msg238959
2015-03-22 16:00:10zbyszsetfiles: + 0001-docs-update-description-of-TemporaryFile-and-tempfil.patch

messages: + msg238916
2015-03-21 04:19:28r.david.murraysetnosy: + r.david.murray
messages: + msg238759
2015-03-21 02:34:02berker.peksagsetnosy: + berker.peksag
title: [PATCH] update tempfile docs to say that TemporaryFile is secure -> update tempfile docs to say that TemporaryFile is secure
messages: + msg238750

versions: + Python 3.4
type: enhancement
2015-03-20 21:44:00ned.deilysetassignee: docs@python

nosy: + docs@python
components: + Documentation, - Library (Lib)
stage: patch review
2015-03-20 19:14:52zbyszcreate