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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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)  |
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) *  |
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.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:58:14 | admin | set | github: 67913 |
2015-08-12 23:41:21 | rbcollins | set | status: open -> closed resolution: fixed messages:
+ msg248490
stage: commit review -> resolved |
2015-08-12 23:38:58 | python-dev | set | nosy:
+ python-dev messages:
+ msg248489
|
2015-08-12 22:37:10 | rbcollins | set | messages:
+ msg248482 |
2015-08-04 02:30:10 | r.david.murray | set | nosy:
+ rhettinger, gregory.p.smith, eric.smith messages:
+ msg247974
|
2015-08-04 00:48:21 | zbysz | set | files:
+ issue-23725.patch
messages:
+ msg247970 |
2015-08-04 00:20:26 | rbcollins | set | messages:
+ msg247968 |
2015-08-03 22:35:22 | zbysz | set | messages:
+ msg247962 |
2015-08-03 22:17:44 | rbcollins | set | messages:
+ msg247959 |
2015-08-03 22:11:54 | r.david.murray | set | messages:
+ msg247958 |
2015-08-03 21:11:07 | rbcollins | set | files:
+ issue-23725.patch nosy:
+ rbcollins messages:
+ msg247954
|
2015-08-03 21:00:31 | rbcollins | set | versions:
+ Python 3.6 |
2015-04-19 17:54:23 | r.david.murray | set | stage: patch review -> commit review |
2015-04-19 00:44:00 | zbysz | set | files:
+ tempfile_docs.patch
messages:
+ msg241467 |
2015-04-18 19:16:31 | r.david.murray | set | messages:
+ msg241448 versions:
- Python 3.4 |
2015-04-18 19:14:36 | r.david.murray | set | messages:
+ msg241447 |
2015-04-18 15:23:05 | zbysz | set | files:
+ tempfile_docs.patch |
2015-04-18 15:19:26 | zbysz | set | messages:
+ msg241420 |
2015-04-15 00:40:57 | r.david.murray | set | messages:
+ msg241063 |
2015-04-15 00:17:35 | r.david.murray | set | files:
+ tempfile_docs.patch
messages:
+ msg241062 |
2015-04-15 00:03:51 | zbysz | set | messages:
+ msg241060 |
2015-03-23 05:03:31 | zbysz | set | files:
+ 0001-docs-update-description-of-TemporaryFile-and-tempfil.patch
messages:
+ msg238980 |
2015-03-23 02:52:45 | martin.panter | set | messages:
+ msg238974 |
2015-03-23 02:16:13 | zbysz | set | files:
+ 0001-docs-update-description-of-TemporaryFile-and-tempfil.patch
messages:
+ msg238972 |
2015-03-23 01:55:45 | r.david.murray | set | messages:
+ msg238970 |
2015-03-22 23:57:49 | martin.panter | set | nosy:
+ martin.panter messages:
+ msg238959
|
2015-03-22 16:00:10 | zbysz | set | files:
+ 0001-docs-update-description-of-TemporaryFile-and-tempfil.patch
messages:
+ msg238916 |
2015-03-21 04:19:28 | r.david.murray | set | nosy:
+ r.david.murray messages:
+ msg238759
|
2015-03-21 02:34:02 | berker.peksag | set | nosy:
+ 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:00 | ned.deily | set | assignee: docs@python
nosy:
+ docs@python components:
+ Documentation, - Library (Lib) stage: patch review |
2015-03-20 19:14:52 | zbysz | create | |