[docs] os.rename documentation slightly inaccurate (issue 16278)

[ezio.melotti at gmail.com]

I left a few comments.
Most of them apply to several parts of the code, even though I just
commented in a single place.


http://bugs.python.org/review/16278/diff/7518/Lib/test/test_os.py
File Lib/test/test_os.py (right):

http://bugs.python.org/review/16278/diff/7518/Lib/test/test_os.py#newcode55
Lib/test/test_os.py:55: # Tests creating TESTFN and os.rename
I think it would be better to make a separate class for os.rename.  The
changes to setUp/tearDown are only needed for the new tests, but they
are executed for all the old tests too, and that's not necessary.

http://bugs.python.org/review/16278/diff/7518/Lib/test/test_os.py#newcode62
Lib/test/test_os.py:62: self.rename_src_file = "%s_%d_tmp" %
("test_rename_src_file", \
All the \ are useless and should be removed (or possibly replaced by an
extra set of ()).

http://bugs.python.org/review/16278/diff/7518/Lib/test/test_os.py#newcode85
Lib/test/test_os.py:85: if (os.path.exists(self.rename_dst_directory +
"/file.txt")):
Shouldn't this use os.path.join()?
Also "/file.txt" is repeated over and over -- it would be better to use
a variable instead.

http://bugs.python.org/review/16278/diff/7518/Lib/test/test_os.py#newcode125
Lib/test/test_os.py:125: os.rmdir(self.rename_src_directory)
This could be rewritten as:
files = [support.TESTFN, self.rename_src_file, ...]
for file in files:
    if os.path.exists(file):
        os.unlink(file) 

for the dirs you can have the same 4 lines with rmdir instead, or check
if they are files or dir and use the same loop.

http://bugs.python.org/review/16278/diff/7518/Lib/test/test_os.py#newcode161
Lib/test/test_os.py:161: @support.cpython_only
Why are this cpython_only?
Does the behavior depend on the CPython implementation or on the OS
and/or underlying posix function?

http://bugs.python.org/review/16278/diff/7518/Lib/test/test_os.py#newcode164
Lib/test/test_os.py:164: (sys.platform == 'win32') ), \
All the \ should be removed, and there are also 4 sets of () that are
not necessary and could be removed as well.

http://bugs.python.org/review/16278/diff/7518/Lib/test/test_os.py#newcode165
Lib/test/test_os.py:165: 'test specific to windows or linux or darwin
only')
Is this because you only tested it on these platforms?
I would run it on other platforms too, and possibly add specific skips
if they fail there (unless of course the failure is caused by a bug that
should be fixed).
If the skip is really necessary I would create a new skip decorator and
use that, instead of repeating this over and over.

http://bugs.python.org/review/16278/diff/7518/Lib/test/test_os.py#newcode167
Lib/test/test_os.py:167: if ((sys.platform == 'darwin') or
(sys.platform.startswith == "linux")):
All the () are unnecessary and should be removed.

http://bugs.python.org/review/16278/diff/7518/Lib/test/test_os.py#newcode171
Lib/test/test_os.py:171: [b"beans", b"bacon", b"eggs", b"spam"])
PEP8 says that this should either be indented as

self.assertEqual(fobj.read().splitlines(),
                 [b"beans", b"bacon", b"eggs", b"spam"])

or as

self.assertEqual(
    fobj.read().splitlines(),
    [b"beans", b"bacon", b"eggs", b"spam"])

http://www.python.org/dev/peps/pep-0008/#indentation

http://bugs.python.org/review/16278/diff/7518/Lib/test/test_os.py#newcode185
Lib/test/test_os.py:185: os.unlink(self.rename_dst_file) # delete
rename_dst_file
Shouldn't the setUp/tearDown already take care of this?

http://bugs.python.org/review/16278/diff/7518/Lib/test/test_os.py#newcode369
Lib/test/test_os.py:369: self.assertRaises(OSError, os.rename,
self.rename_src_file, \
self.assertRaises() can be used as a context manager too, to make the
code more readable.

http://bugs.python.org/review/16278/diff/7518/Lib/test/test_os.py#newcode396
Lib/test/test_os.py:396:
self.assertEqual(os.path.exists(self.rename_dst_directory), True)
self.assertTrue(os.path.exists(self.rename_dst_directory))

http://bugs.python.org/review/16278/