Message99033
Enclosed is patch 21, which addresses the concerns raised by Brian.
Note that I still have work to do resolving the limited user account test failures.
> Brian Curtin <curtin@acm.org> added the comment:
>
> Most of the guts have already been reviewed, but here are some mainly
> minor comments on the last patch.
Thanks again for the thorough review.
> 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
I left `import print_function` in so the script can be run on Python 2.6 without modification.
> 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.
I tried implementing it using decorators, but I'm not confident that stacking decorators will alleviate the need for checking hasattr(sys, 'getwindowsversion') because the decorators will get run even if the previous decorator returns a skip. If this is possible, please propose a solution. Otherwise, the technique written should work (I'll prove it more with subsequent reports).
I did refactor the tests so there's no longer commented code, but there are now two tests that are skipped and will fail if not skipped. I would like to keep these here to capture where functionality may be improved (though it's not obvious to me that these tests necessarily should pass).
> 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.
Done. I removed the TODO. I believe it's an old comment that was already resolved. |
|
Date |
User |
Action |
Args |
2010-02-08 00:35:49 | jaraco | set | recipients:
+ jaraco, lemburg, loewis, nnorwitz, jafo, amaury.forgeotdarc, ggenellina, eric.smith, giampaolo.rodola, swarren, r.david.murray, ssbarnea, brian.curtin, asvetlov |
2010-02-08 00:35:47 | jaraco | link | issue1578269 messages |
2010-02-08 00:35:46 | jaraco | create | |
|