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

[rovitotv at gmail.com]

Ezio,
  Thanks for the review it was excellent.  I got most of them fixed and
will post a new version soon.


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
On 2013/03/04 18:15:16, ezio.melotti wrote:
> 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.
I agree I only put it in the FileTests class because there was a single
test that already existing in that class.  A new class will make it much
cleaner, good suggestion. Will be implemented in next version.

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", \
On 2013/03/04 18:15:16, ezio.melotti wrote:
> All the \ are useless and should be removed (or possibly replaced by
an extra
> set of ()).
I thought those were required but you are right they are not needed so I
removed.

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")):
On 2013/03/04 18:15:16, ezio.melotti wrote:
> Shouldn't this use os.path.join()?
> Also "/file.txt" is repeated over and over -- it would be better to
use a
> variable instead.
I used os.path.join() and I made a variable all good suggestions. 
Thanks!

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)
On 2013/03/04 18:15:16, ezio.melotti wrote:
> 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.
Done.....this is a terrific way to perform the same functions but with
fewer lines of code.  Thank you.

http://bugs.python.org/review/16278/diff/7518/Lib/test/test_os.py#newcode161
Lib/test/test_os.py:161: @support.cpython_only
On 2013/03/04 18:15:16, ezio.melotti wrote:
> Why are this cpython_only?
> Does the behavior depend on the CPython implementation or on the OS
and/or
> underlying posix function?
Nope, the original test case had this so I included it.  But if you
think it is ok I removed it.

http://bugs.python.org/review/16278/diff/7518/Lib/test/test_os.py#newcode164
Lib/test/test_os.py:164: (sys.platform == 'win32') ), \
On 2013/03/04 18:15:16, ezio.melotti wrote:
> All the \ should be removed, and there are also 4 sets of () that are
not
> necessary and could be removed as well.
Ok I removed both the \ and the ().  I personally think the () make code
clearer.

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')
On 2013/03/04 18:15:16, ezio.melotti wrote:
> 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.
I only have access to Mac OS X, Linux, and Windows but would be glad to
try on other platforms if I can get access.  I removed this code for
now.

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")):
On 2013/03/04 18:15:16, ezio.melotti wrote:
> All the () are unnecessary and should be removed.
Done....sorry about that just a personal preference.

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"])
On 2013/03/04 18:15:16, ezio.melotti wrote:
> 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
Yep that is taken care of in the next version.

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
On 2013/03/04 18:15:16, ezio.melotti wrote:
> Shouldn't the setUp/tearDown already take care of this?
setUp/teadDown does in most cases.  But for cases when destination
doesn't exist the destination file must be removed.

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, \
On 2013/03/04 18:15:16, ezio.melotti wrote:
> self.assertRaises() can be used as a context manager too, to make the
code more
> readable.
Ummmm....I will have to research context manager.

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)
On 2013/03/04 18:15:16, ezio.melotti wrote:
> self.assertTrue(os.path.exists(self.rename_dst_directory))
Done....I think the code you suggested is better thanks!

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