[issue23725] update tempfile docs to say that TemporaryFile is secure

[Robert Collins]

Robert Collins added the comment:

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.

--
resolution:  -> fixed
stage: commit review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23725] update tempfile docs to say that TemporaryFile is secure

[Roundup Robot]

Roundup Robot added the comment:

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

--
nosy: +python-dev

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23725] update tempfile docs to say that TemporaryFile is secure

[Robert Collins]

Robert Collins added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23725] update tempfile docs to say that TemporaryFile is secure

[R. David Murray]

R. David Murray added the comment:

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.

--
nosy: +eric.smith, gregory.p.smith, rhettinger

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23725] update tempfile docs to say that TemporaryFile is secure

[Zbyszek Jędrzejewski-Szmek]

Zbyszek Jędrzejewski-Szmek added the comment:

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

--
Added file: http://bugs.python.org/file40122/issue-23725.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23725] update tempfile docs to say that TemporaryFile is secure

[Robert Collins]

Robert Collins added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23725] update tempfile docs to say that TemporaryFile is secure

[Zbyszek Jędrzejewski-Szmek]

Zbyszek Jędrzejewski-Szmek added the comment:

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".

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23725] update tempfile docs to say that TemporaryFile is secure

[Robert Collins]

Robert Collins added the comment:

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?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23725] update tempfile docs to say that TemporaryFile is secure

[R. David Murray]

R. David Murray added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23725] update tempfile docs to say that TemporaryFile is secure

[Robert Collins]

Robert Collins added the comment:

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.

--
nosy: +rbcollins
Added file: http://bugs.python.org/file40120/issue-23725.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23725] update tempfile docs to say that TemporaryFile is secure

[Robert Collins]

Changes by Robert Collins :


--
versions: +Python 3.6

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23725] update tempfile docs to say that TemporaryFile is secure

[R. David Murray]

Changes by R. David Murray :


--
stage: patch review -> commit review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23725] update tempfile docs to say that TemporaryFile is secure

[Zbyszek Jędrzejewski-Szmek]

Zbyszek Jędrzejewski-Szmek added the comment:

v6:
- add newline

--
Added file: http://bugs.python.org/file39112/tempfile_docs.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23725] update tempfile docs to say that TemporaryFile is secure

[R. David Murray]

R. David Murray added the comment:

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*.

--
versions:  -Python 3.4

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23725] update tempfile docs to say that TemporaryFile is secure

[R. David Murray]

R. David Murray added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23725] update tempfile docs to say that TemporaryFile is secure

[Zbyszek Jędrzejewski-Szmek]

Changes by Zbyszek Jędrzejewski-Szmek :


Added file: http://bugs.python.org/file39104/tempfile_docs.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23725] update tempfile docs to say that TemporaryFile is secure

[Zbyszek Jędrzejewski-Szmek]

Zbyszek Jędrzejewski-Szmek added the comment:

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'

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23725] update tempfile docs to say that TemporaryFile is secure

[R. David Murray]

R. David Murray added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23725] update tempfile docs to say that TemporaryFile is secure

[R. David Murray]

R. David Murray added the comment:

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

--
Added file: http://bugs.python.org/file39026/tempfile_docs.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23725] update tempfile docs to say that TemporaryFile is secure

[Zbyszek Jędrzejewski-Szmek]

Zbyszek Jędrzejewski-Szmek added the comment:

Ping?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23725] update tempfile docs to say that TemporaryFile is secure

[Zbyszek Jędrzejewski-Szmek]

Zbyszek Jędrzejewski-Szmek added the comment:

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

v4:
- one more "and"

--
Added file: 
http://bugs.python.org/file38643/0001-docs-update-description-of-TemporaryFile-and-tempfil.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23725] update tempfile docs to say that TemporaryFile is secure

[Martin Panter]

Martin Panter added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23725] update tempfile docs to say that TemporaryFile is secure

[Zbyszek Jędrzejewski-Szmek]

Zbyszek Jędrzejewski-Szmek added the comment:

> 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

--
Added file: 
http://bugs.python.org/file38640/0001-docs-update-description-of-TemporaryFile-and-tempfil.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23725] update tempfile docs to say that TemporaryFile is secure

[R. David Murray]

R. David Murray added the comment:

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 ;)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23725] update tempfile docs to say that TemporaryFile is secure

[Martin Panter]

Martin Panter added the comment:

[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 . . .”

--
nosy: +vadmium

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23725] update tempfile docs to say that TemporaryFile is secure

[Zbyszek Jędrzejewski-Szmek]

Zbyszek Jędrzejewski-Szmek added the comment:

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

--
Added file: 
http://bugs.python.org/file38634/0001-docs-update-description-of-TemporaryFile-and-tempfil.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23725] update tempfile docs to say that TemporaryFile is secure

[R. David Murray]

R. David Murray added the comment:

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

--
nosy: +r.david.murray

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23725] update tempfile docs to say that TemporaryFile is secure

[Berker Peksag]

Berker Peksag added the comment:

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.)

--
nosy: +berker.peksag
title: [PATCH] update tempfile docs to say that TemporaryFile is secure -> 
update tempfile docs to say that TemporaryFile is secure
type:  -> enhancement
versions: +Python 3.4

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com