classification
Title: Improve os.rename documentation and tests
Type: enhancement Stage:
Components: Documentation, Library (Lib), Tests Versions: Python 3.4
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: David.Benjamin, Todd.Rovito, asvetlov, benjamin.peterson, chris.jerdonek, docs@python, ezio.melotti, loewis, terry.reedy
Priority: normal Keywords: patch

Created on 2012-10-18 17:36 by David.Benjamin, last changed 2014-04-14 21:46 by orsenthil.

Files
File name Uploaded Description Edit
OSRenameCombinations.py Todd.Rovito, 2012-11-07 05:17
16278OSRenameDocsTest.patch Todd.Rovito, 2013-03-04 03:53 review
16278OSRenameDocsTestV2.patch Todd.Rovito, 2013-03-10 03:36 review
16278OSRenameDocsTestV3.patch Todd.Rovito, 2013-03-11 02:18 review
16278OSRenameDocsTestV4.patch Todd.Rovito, 2013-03-11 02:46 review
16278OSRenameDocsTestV5.patch Todd.Rovito, 2014-02-16 05:11 review
Messages (36)
msg173285 - (view) Author: David Benjamin (David.Benjamin) Date: 2012-10-18 17:36
This is somewhat of a nitpick. os.rename's documentation says "If dst is a directory, OSError will be raised". On Unix, this isn't completely true. If the source is a directory and the destination is an empty directory, it'll overwrite the former with the latter. (Actually if the source is a directory the inverse is true; if dst is a file, OSError will be raised.)

In [1]: import os

In [2]: os.mkdir("aaa")

In [3]: open("aaa/blah", "w").close()

In [4]: os.mkdir("bbb")

In [5]: os.rename("aaa", "bbb")

In [6]: os.listdir("bbb")
Out[6]: ['blah']
msg173410 - (view) Author: Todd Rovito (Todd.Rovito) * Date: 2012-10-20 20:24
David,
   Thanks for your bug report. Indeed os.rename does not exhibit the same behavior as the documentation describes. 

For Python 3.4 here is the fix I came up with:
"Rename the file or directory src to dst. If dst is a directory that is not empty, OSError will be raised. On Unix, if dst exists and is a file, it will be replaced silently if the user has permission and src is a file. On Unix, if src is a directory and dst is a file NotADirectoryError will be raised. The operation may fail on some Unix flavors if src and dst are on different filesystems. If successful, the renaming will be an atomic operation (this is a POSIX requirement). On Windows, if dst already exists, OSError will be raised even if it is a file."

I have attached a Python 3.4 patch for consideration.  This might not be the best phrasing so please feel free to offer alternatives.
msg173715 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-10-24 22:25
Chris, maybe you can guess better wording?
msg173729 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-10-25 03:22
Before we discuss wording, it's not clear to me whether all possibilities have been checked.  I count 2*3*2=12 possibilities to check (excluding src and dst being on different filesystems):

src: file or directory
dst: file, empty directory, or non-empty directory
os:  Unix or Windows

Also, do we have tests for all of these scenarios before we document it?
msg173820 - (view) Author: Todd Rovito (Todd.Rovito) * Date: 2012-10-26 05:32
Thanks for the feedback!  Over the weekend I will make sure the documentation and test cases cover all possibilities.  I have not worked with test suite but I will do my best.
msg174180 - (view) Author: Todd Rovito (Todd.Rovito) * Date: 2012-10-30 04:33
Over the weekend I verified the test cases are incomplete for Python 3.4.  Inside of Lib/test/test_os.py is a class FileTests that contains a single function test_rename which seems to only check to make sure that the reference count for the first argument is not mis-handeled.  Nothing exists for the use case destination is a directory as described in the original bug.  I will attempt to create suggested test cases but as I am not the most familiar with Python's unit test framework it might take me some time, I estimate as long as 11/6/2012.  If somebody comes along with more experience that can fix the bug please feel free to proceed.
msg174259 - (view) Author: Todd Rovito (Todd.Rovito) * Date: 2012-10-31 04:03
While writing test cases I discovered another conflict with the documentation.  The phrase "On Unix, if dst exists and is a file, it will be replaced silently if the user has permission and src is a file." is not correct.  According to the test cases I wrote in the attached patch see the method test_rename_src_file_dest_file_exists in the class FileTests the destination file is not replaced and the source file actually is removed.  I will seek advice from Python Core Mentorship to see if this is a bug or normal behavior. 

First time writing test cases so I hope I am on the right track.
msg174265 - (view) Author: Todd Rovito (Todd.Rovito) * Date: 2012-10-31 05:10
False alarm my test case was buggy.
msg174861 - (view) Author: Todd Rovito (Todd.Rovito) * Date: 2012-11-05 04:09
Attached is a patch for 16 test cases.  All 16 test cases have been tested on Windows 7, Mac OS X, and Linux they seem to function well.  Before this patch there was only a single test case for rename.  For each test case I used "unittest.skipUnless" to make sure the platform was one of the three that I tested for Windows 7, Mac OS X, or Linux.  These test cases demonstrate that the documentation is incorrect and perhaps a little fuzzy.  I plan to submit updates to the documentation in the coming days.  This patch is only for Python 3.4 but I will backport to 2.7 in a few days.  


I wrote the following code to find all the combinations based on parameters of the function:

src = ['src_file','src_directory_empty', 'src_directory_not_empty', \
       'src_file_or_directory_not_exist']
dst = ['dst_file_exist','dst_not_exist','dst_directory_empty', \
        'dst_directory_not_empty']

print "Make sure you have functions in test_os for all of these"
for index_src in range(0, len(src)):
    for index_dst in range(0, len(dst)):
        function_name = "test_rename_" + src[index_src] + "_" + \
            dst[index_dst]
        print function_name
msg174862 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-11-05 04:46
> Attached is a patch for 16 test cases.

The test cases look quite verbose (e.g. they're not DRY), but it's a good start.  Thanks.  For others' benefit, can you perhaps summarize your findings concisely in a table/chart of some form?
msg175043 - (view) Author: Todd Rovito (Todd.Rovito) * Date: 2012-11-07 05:17
Chris,
   That is an excellent suggestion.  I modified my OSRenameCombinations.py program and attached.  This program prints a table with the src parameters as columns and the dst as rows.  Hopefully it will show up ok in the bug tracker.


For Unix
                              src_file                       src_directory_empty            src_directory_not_empty        src_file_or_directory_not_exist 
dst_file_exist                 rename overwrite               raises OSError                 raises OSError                 raises OSError                 
dst_not_exist                  rename                         rename                         rename                         raises OSError                 
dst_directory_empty            raises OSError                 rename overwrite               rename overwrite               raises OSError                 
dst_directory_not_empty        raises OSError                 raises OSError                 raises OSError                 raises OSError                 
For Windows
                              src_file                       src_directory_empty            src_directory_not_empty        src_file_or_directory_not_exist 
dst_file_exist                 raises OSError                 raises OSError                 raises OSError                 raises OSError                 
dst_not_exist                  rename                         rename                         rename                         raises OSError                 
dst_directory_empty            raises OSError                 raises FileExistsError         raises FileExistsError         raises OSError                 
dst_directory_not_empty        raises OSError                 raises OSError                 raises OSError                 raises OSError
msg175133 - (view) Author: Todd Rovito (Todd.Rovito) * Date: 2012-11-08 04:01
Here is a draft suggestion for the documentation change, not all the formatting is worked out:

.. function:: rename(src, dst, *, src_dir_fd=None, dst_dir_fd=None)

   Rename the file or directory *src* to *dst*. If *src* exists as either
   a file or directory and *dst* does not exist the rename will occur with
   no error raised.  In some cases the rename function will behave
   differently across platforms which are noted below. In all cases
   if *src* does not exist :exc: 'OSError' will be raised.

   Unix
   If *dst* exists and is a file, it will be replaced silently if the user
   has permission and src is a file.  If *src* is a directory and *dst* is a
   file :exc: 'OSError' will be raised.  In the case where *src* is a
   directory and *dst* is a empty directory the rename will occur and the
   *src* directory name will overwrite the *dst* directory name.Yet a special
   case is noted where *src* is a directory and *dst* is a non-empty directory
   the rename will not occur and :exc: `OSError` will be raised.  

   Windows
   If *src* is a file and *dst* exists either as a file or directory :exc:
   'OSError` will be raised and the rename will not occur.  In the case where
   *src* is a directory, either empty or not empty, and *dst* exists as a
   file or not empty directory :exc: `OSError` will be raised.  If *src* is
   a directory, either empty or not empty, and *dst* is a empty directory
   then :exc: `FileExistsError` will be raised.
        

   If successful, the renaming will be an atomic operation (this is a POSIX
   requirement).
   
   This function can support specifying *src_dir_fd* and/or *dst_dir_fd* to
   supply :ref:`paths relative to directory descriptors <dir_fd>`.

   If you want cross-platform overwriting of the destination, use
   :func:`replace`.

   Availability: Unix, Windows.

   .. versionadded:: 3.3
      The *src_dir_fd* and *dst_dir_fd* arguments.
msg176095 - (view) Author: Todd Rovito (Todd.Rovito) * Date: 2012-11-22 04:58
Attached is patch with the final formatting for the documentation updates. I fixed the :exc:`OSError` problems that I had before and used indents to denote Unix behavior VS Windows behavior. Please let me know if I can do anything else to help get this issue resolved.  Thanks!
msg181991 - (view) Author: Todd Rovito (Todd.Rovito) * Date: 2013-02-13 03:33
This is a gentle ping of this issue.  Can somebody please review and let me know what needs to be done to get this committed?  I did test the patch on 2/12/2013 and it seems to work from the latest 3.4.  Thanks!
msg182003 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2013-02-13 08:05
The doc change looks good to me. I am adding Terry and Ezio to see if they have any comments on wordings in the doc. If not, I can commit this change. I think that this is helpful.
msg182004 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2013-02-13 08:10
I commented above that the tests are not very DRY though.  Shouldn't there be tests to check that the documented behavior is correct?
msg182005 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2013-02-13 08:43
Chris:  The patch is for the docs. the test code I believe, is for
reference. It would be to run it on different OS and verify the
documentation matches the behavior.I am okay with more than one person
verifying this and I shall do on OS'es I have.
msg182041 - (view) Author: Todd Rovito (Todd.Rovito) * Date: 2013-02-13 15:31
Chris,
   I first verified the issue then created some wording and you pointed out that we needed test cases so I created a bunch of test cases.  As you pointed out "I count 2*3*2=12 possibilities to check (excluding src and dst being on different filesystems):" so I created 16 test cases then discovered that the behavior is different on Windows and Unix so all together there are 32 test cases.  Your last comment about the test cases being "Dry" didn't make sense to me.  If you provide specific feedback I would be glad to work on the issue.  The test cases are designed to make sure that the documented behavior is actually how Python performs.  It made the most sense to me to put together two patches one for the test cases and another for the documentation, but maybe that has confused everybody?  If it would help I could submit a single patch.  OSRenameCombinations.py was just for reference to prove to myself that I got all the possible test cases.

I am a fairly new Python contributor and this is my first attempt at writing test cases so I could be missing something.  Thanks for the mentorship and the time required for a little extra guidance.
msg182050 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2013-02-13 17:08
Senthil, in my experience, whenever documentation is added that documents new aspects of behavior (e.g. is not just a rewording of existing documentation), tests are always added simultaneously (if not already present) to ensure that the code doesn't regress against the new documentation.

Todd, DRY means "don't repeat yourself."  You can look it up on Wikipedia, etc.  Identical chunks of code are repeated several times which make it harder for others to see how the test cases differ from each other (as well as making the code harder to maintain).
msg182051 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2013-02-13 17:13
> If it would help I could submit a single patch.

Yes, a single patch is best.  One way to make code more DRY is refactoring to use helper functions as needed (i.e. same code in multiple places -> one helper function).
msg182057 - (view) Author: Todd Rovito (Todd.Rovito) * Date: 2013-02-13 18:59
Chris,
  Thanks for the clarification.  I thought you were telling me my test cases were dry as in dry humor....I will read-up on the dry concept and see what I can do to consolidate plus I will combine the patches.  Initially I am thinking I could collapse all the test cases into two functions each for their respective operating system.  Would that be ok?
msg182058 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2013-02-13 19:09
The number of test cases isn't the problem.  Having more but finer-grained test cases is usually preferred in fact.  It's just that the test cases should be sharing code where possible so that they're shorter and easy to see how they differ from one another.
msg182074 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-02-14 04:56
"directory name.Yet a" needs spaces after '.'.

The text is decent English and clear enough sentence by sentence, but the reality and hence the text as a whole is confusing.

I wonder if it would be possible to make a little table

                    dest
                    ----   
Src     file         empty-dir    non-empty-dir
----    -----------------------------------------
file |            |            |

dir

with the behavior for each combination indicated, 'success' or 'OSError', with two lines prefixed with Unix, Win where they differ. Then the differences would be much more obvious.

(A separate issue: Patch says that Windows currently raises a different error in one situation. I think it would be better -- in a future version -- if that were caught and reraised as OSError also.)
msg182094 - (view) Author: Todd Rovito (Todd.Rovito) * Date: 2013-02-14 12:39
Thanks Terry and Chris you guys have supplied great feedback.  I will work on the issue and try to get a new patch updated by end of the weekend (2/18/13).

Sent from my iPhone

On Feb 13, 2013, at 11:56 PM, "Terry J. Reedy" <report@bugs.python.org> wrote:

> 
> Terry J. Reedy added the comment:
> 
> "directory name.Yet a" needs spaces after '.'.
> 
> The text is decent English and clear enough sentence by sentence, but the reality and hence the text as a whole is confusing.
> 
> I wonder if it would be possible to make a little table
> 
>                    dest
>                    ----   
> Src     file         empty-dir    non-empty-dir
> ----    -----------------------------------------
> file |            |            |
> 
> dir
> 
> with the behavior for each combination indicated, 'success' or 'OSError', with two lines prefixed with Unix, Win where they differ. Then the differences would be much more obvious.
> 
> (A separate issue: Patch says that Windows currently raises a different error in one situation. I think it would be better -- in a future version -- if that were caught and reraised as OSError also.)
> 
> ----------
> 
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue16278>
> _______________________________________
msg182355 - (view) Author: Todd Rovito (Todd.Rovito) * Date: 2013-02-19 01:02
Over the weekend I caught this terrible cold and have not been able to work on this issue much.  Hopefully I can complete by next weekend 2/25/2013 thanks for your understanding.
msg183416 - (view) Author: Todd Rovito (Todd.Rovito) * Date: 2013-03-04 03:53
Combined the test cases and document changes into a single patch.  As suggested by Mr. Terry Reedy I used a table and removed the text.  The table still needs some work but it is a good start.  As suggested by Mr. Jerdonek I tried to make the test cases WETter by setting up the files and directories one time in the setup method then only changing that setup in the actual test case only if needed.  

This patch needs still needs some work but I wanted people to know I had not quit yet.  It still needs to be tested on Windows, which I will do ASAP.  Thanks.
msg183477 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2013-03-04 17:27
Adding more tests is good, even though there are still a few things that should be improved (see comments on rietveld).
Regarding the documentation I'm not sure it's a good idea to be so detailed.  On one hand, if we test the behavior we can make sure that the documentation is accurate, OTOH it might make the docs more confusing and once this behavior is documented it will be difficult to change it (and there might still be exceptions on different platforms/filesystems or if symlinks are involved).
Maybe it would be better to suggest a LBYL approach rather trying to document and deal with all the different combinations.
msg183508 - (view) Author: Todd Rovito (Todd.Rovito) * Date: 2013-03-05 04:05
Ezio,
  Thank you or the feedback I will continue to polish the test cases.  As far as the documentation I really like the table and find it easy to read but I could go either way.  If you read the history of this issue other Python Core Developers asked to write test cases and make sure the documentation matches the test cases.  It makes no difference to me I just want to contribute!!!!  Which approach will produce the best results for the Python community? I think the table is a good compromise.
msg183865 - (view) Author: Todd Rovito (Todd.Rovito) * Date: 2013-03-10 03:36
Version 2 of the patch includes many improvements most of which were suggested by Ezio making the patch a much higher quality.  The patch is very WET at this point.  I still need to test on Windows which I plan to do as soon as I can get my Windows partition in order.  

Thanks for all the great feedback!
msg183911 - (view) Author: Todd Rovito (Todd.Rovito) * Date: 2013-03-11 02:18
V3 added which includes much shorter variable names and a cleanup of the patch.  I still need to test on Windows.
msg183912 - (view) Author: Todd Rovito (Todd.Rovito) * Date: 2013-03-11 02:21
Thanks for the feedback Vitaliy Stepanov that helped alot with the version 3 patch.
msg183915 - (view) Author: Todd Rovito (Todd.Rovito) * Date: 2013-03-11 02:46
Made some very minor changes to get patch to work on Windows.  Had issues with line ending differences between Unix and Windows. I fixed it so line endings won't be an issue and tested patch on Windows, Linux, and OS X.  

Please feel free to supply feedback or commit.  Thanks.
msg190125 - (view) Author: Todd Rovito (Todd.Rovito) * Date: 2013-05-27 02:24
Ping!!!

I have not heard anything about this patch so I wanted to ping it to get more feedback.  Thanks!
msg211305 - (view) Author: Todd Rovito (Todd.Rovito) * Date: 2014-02-16 05:11
Retested this patch with the latest 3.4 and made one tiny change to get it to apply cleanly.  Please provide feedback or commit.  I would like to get this committed after more than a year since the original bug report.
msg211306 - (view) Author: Todd Rovito (Todd.Rovito) * Date: 2014-02-16 05:13
I forgot to mention this patch only works on 3.4 but if it is committed I will work on a patch for 2.7.  Thanks.
msg211427 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2014-02-17 17:16
1. You removed the note about files being on the same filesystem on Unix. That's useful.
2. I don't think it needs to be mentioned that you'll get an error if *src* doesn't exist.
3. The table is strange because the "destination" header spans 2 columns, while the description of destination types seems to be 3 columns.
4. The destination type entries are marked as the table header, but the source type entries are not.
History
Date User Action Args
2014-04-14 21:46:48orsenthilsetnosy: - orsenthil
2014-02-17 17:16:21benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg211427
2014-02-16 16:27:00terry.reedysetassignee: docs@python ->
title: os.rename documentation slightly inaccurate -> Improve os.rename documentation and tests
nosy: + loewis
components: + Library (Lib), Tests
2014-02-16 05:13:43Todd.Rovitosetmessages: + msg211306
2014-02-16 05:11:20Todd.Rovitosetfiles: + 16278OSRenameDocsTestV5.patch

messages: + msg211305
2013-05-27 02:24:55Todd.Rovitosetmessages: + msg190125
2013-03-11 02:46:42Todd.Rovitosetfiles: + 16278OSRenameDocsTestV4.patch

messages: + msg183915
2013-03-11 02:21:24Todd.Rovitosetmessages: + msg183912
2013-03-11 02:18:14Todd.Rovitosetfiles: + 16278OSRenameDocsTestV3.patch

messages: + msg183911
2013-03-10 03:36:24Todd.Rovitosetfiles: + 16278OSRenameDocsTestV2.patch

messages: + msg183865
2013-03-05 04:05:14Todd.Rovitosetmessages: + msg183508
2013-03-04 17:27:47ezio.melottisetmessages: + msg183477
2013-03-04 03:53:54Todd.Rovitosetfiles: + 16278OSRenameDocsTest.patch

messages: + msg183416
2013-03-04 03:48:54Todd.Rovitosetfiles: - OSRenameDocs3point4.patch
2013-03-04 03:48:42Todd.Rovitosetfiles: - OSRename_test_os_3point4.patch
2013-02-19 01:02:56Todd.Rovitosetmessages: + msg182355
2013-02-14 12:39:28Todd.Rovitosetmessages: + msg182094
2013-02-14 04:56:02terry.reedysetmessages: + msg182074
2013-02-13 19:09:53chris.jerdoneksetmessages: + msg182058
2013-02-13 18:59:13Todd.Rovitosetmessages: + msg182057
2013-02-13 17:13:51chris.jerdoneksetmessages: + msg182051
2013-02-13 17:08:08chris.jerdoneksetmessages: + msg182050
2013-02-13 15:31:08Todd.Rovitosetmessages: + msg182041
2013-02-13 08:43:42orsenthilsetmessages: + msg182005
2013-02-13 08:10:21chris.jerdoneksetmessages: + msg182004
2013-02-13 08:05:37orsenthilsetnosy: + ezio.melotti, terry.reedy, orsenthil
messages: + msg182003
2013-02-13 03:33:15Todd.Rovitosetmessages: + msg181991
2012-11-22 05:00:35Todd.Rovitosetfiles: - OSRenameTest3point4.patch
2012-11-22 04:58:29Todd.Rovitosetfiles: + OSRenameDocs3point4.patch

messages: + msg176095
2012-11-08 04:01:19Todd.Rovitosetmessages: + msg175133
2012-11-07 05:17:26Todd.Rovitosetfiles: + OSRenameCombinations.py

messages: + msg175043
2012-11-05 04:46:07chris.jerdoneksetmessages: + msg174862
2012-11-05 04:09:31Todd.Rovitosetfiles: + OSRename_test_os_3point4.patch

messages: + msg174861
versions: - Python 2.7, Python 3.2, Python 3.3
2012-10-31 05:10:44Todd.Rovitosetmessages: + msg174265
2012-10-31 05:10:02Todd.Rovitosetfiles: - OSRename_test_os.patch
2012-10-31 04:03:05Todd.Rovitosetfiles: + OSRename_test_os.patch

messages: + msg174259
2012-10-30 04:33:29Todd.Rovitosetmessages: + msg174180
2012-10-26 05:32:12Todd.Rovitosetmessages: + msg173820
2012-10-25 03:22:49chris.jerdoneksetmessages: + msg173729
2012-10-24 22:25:23asvetlovsetnosy: + chris.jerdonek
messages: + msg173715
2012-10-24 15:47:57asvetlovsetnosy: + asvetlov
2012-10-20 20:24:41Todd.Rovitosetfiles: + OSRenameTest3point4.patch
keywords: + patch
messages: + msg173410
2012-10-20 06:51:04Todd.Rovitosetnosy: + Todd.Rovito
2012-10-18 17:53:40serhiy.storchakasetversions: + Python 3.2, Python 3.3, - Python 3.5
2012-10-18 17:36:16David.Benjamincreate