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

rovitotv at gmail.com rovitotv at gmail.com
Mon Mar 11 03:15:07 CET 2013


Thanks for the review...I think the patch is getting better and better. 
Soon I should have this tested on Windows, hopefully tonight.


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

http://bugs.python.org/review/16278/diff/7565/Lib/test/test_os.py#newcode2209
Lib/test/test_os.py:2209: class RenameTests(unittest.TestCase):
On 2013/03/11 00:45:35, vstepanov wrote:
> I have some comments to this patch.
> I think would be better to add two separate test case RenameFileTests
and
> RenameDirTests or something like that.
I actually like it in a single class because alot of the functionality
is the same for testing the rename of a directory or file.  If we
separate it into two classes then I am afraid that we get lots of
repeating which leads to DRY test cases.  See earlier comments in issue.

http://bugs.python.org/review/16278/diff/7565/Lib/test/test_os.py#newcode2215
Lib/test/test_os.py:2215: self.rename_src_file = "%s_%d_tmp" %
("test_rename_src_file",
On 2013/03/11 00:45:35, vstepanov wrote:
> All variables has prefix ``rename_`` (rename_src_file, rename_dst_file
and so
> on), so can use them without this prefix (just src_file, dst_file ...)
Good point....this is a side effect from me moving over to a new class
before it was in the FileTests directory.  Now that we have a
RenameTests class we can remove the rename_ in front of all the
variables this will help readability.  Thanks!

http://bugs.python.org/review/16278/diff/7565/Lib/test/test_os.py#newcode2216
Lib/test/test_os.py:2216: os.getpid())
On 2013/03/11 00:45:35, vstepanov wrote:
> should not call this method everywhere, use a variable
Yes that will be more efficient, thanks!

http://bugs.python.org/review/16278/diff/7565/Lib/test/test_os.py#newcode2217
Lib/test/test_os.py:2217: if os.path.exists(self.rename_src_file):
On 2013/03/11 00:45:35, vstepanov wrote:
> There is remove_file_or_directory method, use it for remove exist
files/dirs.
Good point, I don't know how I missed the remove_file_or_directory in
setUp but I did.  Thanks.

http://bugs.python.org/review/16278/diff/7565/Lib/test/test_os.py#newcode2220
Lib/test/test_os.py:2220: os.write(fd, b"beans\n")
On 2013/03/11 00:45:35, vstepanov wrote:
> Really need to write many lines in a file and then split the text on
the line in
> tests? Perhaps enough only one.
I got the original code from test_os.py line 111 in the TestWrite
method.  But it does not have to be that way I think the TestWrite
method was testing writes with binary data, byte array data, and memory
view data.  This is actually easier to read and doing the split lines
doesn't add any value.  Thanks.

http://bugs.python.org/review/16278/diff/7565/Lib/test/test_os.py#newcode2277
Lib/test/test_os.py:2277: if (os.path.isfile(file)):
On 2013/03/11 00:45:35, vstepanov wrote:
> outer parentheses are unnecessary
Yes too much scheme sorry I missed those, they are gone now.

http://bugs.python.org/review/16278/diff/7565/Lib/test/test_os.py#newcode2289
Lib/test/test_os.py:2289: self.assertRaises(OSError, os.rename, src,
dst)
On 2013/03/11 00:45:35, vstepanov wrote:
> I do not see where `src` and `dst` variables are declared
Oops I didn't catch that because I have not tested on Windows yet.  I do
plan to test on Windows soon maybe even tonight.  My Windows partition
is not setup correctly yet since I got my new computer back in October.

http://bugs.python.org/review/16278/diff/7565/Lib/test/test_os.py#newcode2295
Lib/test/test_os.py:2295:
self.remove_file_or_directory(self.rename_dst_file)
On 2013/03/11 00:45:35, vstepanov wrote:
> I think it would be better to create the files/directories in separate
methods
> and call them in tests methods instead of create them in setUp method
and remove
> in each test (except for those that are not removed in tests methods,
they can
> be left in the setUp).
I have been wondering if there is a better approach to all of this. 
According to some of the CPython Committers we have to make the test
case as WET as possible and not repeat ourselves.  Before I was actually
creating the files/directories in the test cases but was not using a
method it took a lot of code.  I could make a version that does that but
use methods to create the files instead then measure the line count to
see which solution is WETTEST.  Let me think about this....

http://bugs.python.org/review/16278/diff/7565/Lib/test/test_os.py#newcode2326
Lib/test/test_os.py:2326:
self.assertEqual(os.path.exists(self.rename_dst_directory), True)
On 2013/03/11 00:45:35, vstepanov wrote:
> There are unittest.TestCase.assertTrue and
unittest.TestCase.assertFalse
> methods.
Yes I fixed some of them with V2 but V3 now has all of them fixed. 
Thanks.

http://bugs.python.org/review/16278/diff/7565/Lib/test/test_os.py#newcode2402
Lib/test/test_os.py:2402: src = self.rename_src_file + "does_not_exist"
On 2013/03/11 00:45:35, vstepanov wrote:
> I don't see where `src` and `dst` are used.
You are correct I don't need those thanks!


I will post a new version soon....

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


More information about the docs mailing list