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']

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.

Chris, maybe you can guess better wording?

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?

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.

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.

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.

False alarm my test case was buggy.

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

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?

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

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:
'OSErrorwill 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:OSErrorwill 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.

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!

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!

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.

I commented above that the tests are not very DRY though. Shouldn't there be tests to check that the documented behavior is correct?

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.

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.

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

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

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?

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.

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

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\>

---

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.

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.

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.

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.

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!

V3 added which includes much shorter variable names and a cleanup of the patch. I still need to test on Windows.

Thanks for the feedback Vitaliy Stepanov that helped alot with the version 3 patch.

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.

Ping!!!

I have not heard anything about this patch so I wanted to ping it to get more feedback. Thanks!

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.

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.

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.

In the current doc all cases seem to be covered, but just not in a table format. Is that something that might still be wanted?