Brian Curtin <cur...@acm.org> added the comment: Most of the guts have already been reviewed, but here are some mainly minor comments on the last patch.
Lib/test/symlink_support.py - No need to import print_function - I'd just put the docstrings on one line - A bunch of if tests are one-lined Lib/test/test_win_symlink.py - getwindowsversion now returns named members, so you can just use getwindowsversion().major for example, rather than the tuple. - Rather than check the Windows version in the setUp, I think it would be cleaner to use the unittest.skipIf decorator on the entire WindowsSymlinkTest class. - You might want to stack skip decorators to first skip outright if it's not Windows, and then skip if it's not Win 6 or greater -- it gets you out of checking if getwindowsversion actually exists. - test_remove_directory_link_to_missing_target has some commented out code. Lib/test/test_posixpath.py - Lines 365-366 - why check has_symlink() and immediately check it again? Modules/posixmodule.c - Line 2740 - "//*** todo: fix this" - C++ style comments in a few other places (nit picky, I know) - Line 5117 - win_symlink__doc__ is double spaced and contains an extra slash at line 5121. For consistency it should probably just be single spaced. ---------- _______________________________________ Python tracker <rep...@bugs.python.org> <http://bugs.python.org/issue1578269> _______________________________________ _______________________________________________ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
