Ray.Allen <ysj....@gmail.com> added the comment:

Thanks for reviewing!

> The name 'ensure_exist' for the new parameter strikes me as wrong, as well as 
> too long for something that will nearly always be set to True. The function 
> always ensures that the directory exists. The question is whether it is ok 
> that it exist previously. I strongly suggest something shorter like 
> 'exist_ok' as an alternative.


Yes, I guess you'are right, this parameter needs to be shorter.


> Does the use of 'base = support.TESTFN' ensure that the test junk gets 
> cleaned up?

Yes, the MakedirTests.tearDown() method will try to look for the outermost 
directory of "@base/dir1/dir2/dir3/dir4/dir5/dir6", so it can ensure the test 
directory is cleaned up.


> * Location of error suppression: this patches posixfile.mkdir; 1675 wraps it 
> with a new os.mkdir function that does the suppression. I can see an argument 
> for each approach.

The purpose of posix_mkdir() is to simulate a shell mkdir command, so I think 
the function of "-p" option should goes in to this code. A wrapper makes code 
more complicated.


> * Propagation to parent directories: this passes exist_ok to parent mkdir(); 
> 1675 passes exist_ok=True, so that it is never an error for parent 
> directories to exist. This is a change in behavior and might be bad for the 
> same reason we do not make exist_ok=True the default. In any case, I believe 
> either patch could be changed to mimic the other.

It seems these two ways has the same effect, I have no opinion on which to 
prefer.


By adopting your parameter naming suggestion and a little coding style change, 
I update the patch.

----------
Added file: http://bugs.python.org/file18075/mkdir_py3k_updated.diff

_______________________________________
Python tracker <rep...@bugs.python.org>
<http://bugs.python.org/issue9299>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

Reply via email to