Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(167037)

#16278: os.rename documentation slightly inaccurate

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years, 1 month ago by davidben
Modified:
5 years, 10 months ago
Reviewers:
ezio.melotti, rovitotv, vitaliy.stepanov
CC:
loewis, terry.reedy, Benjamin Peterson, ezio.melotti, asvetlov, cjerdonek, docs_python.org, Todd.Rovito, davidben_davidben.net
Visibility:
Public.

Patch Set 1 #

Patch Set 2 #

Patch Set 3 #

Patch Set 4 #

Patch Set 5 #

Total comments: 24

Patch Set 6 #

Total comments: 20

Patch Set 7 #

Patch Set 8 #

Patch Set 9 #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Doc/library/os.rst View 1 2 3 4 5 6 7 8 1 chunk +24 lines, -8 lines 0 comments Download
Lib/test/test_os.py View 1 2 3 4 5 6 7 8 3 chunks +207 lines, -0 lines 0 comments Download

Messages

Total messages: 5
ezio.melotti
I left a few comments. Most of them apply to several parts of the code, ...
6 years, 9 months ago #1
rovitotv_gmail.com
Ezio, Thanks for the review it was excellent. I got most of them fixed and ...
6 years, 9 months ago #2
vstepanov
some comments to the patch 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): I have ...
6 years, 9 months ago #3
vstepanov
some comments to the patch
6 years, 9 months ago #4
rovitotv_gmail.com
6 years, 9 months ago #5
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....
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7+