Message48929
Logged In: YES
user_id=33168
Thanks for the patch, it's not in bad shape. Please attach
the doc patch file here. It's easier to have a single patch
than multiple.
A couple of things about the patch. Some of these should be
in PEP 8.
* <> is deprecated in favor of != (didn't see this doc'd in
PEP 8 though, maybe it's in another PEP)
* don't add extra parentheses, e.g.,
(abspath(start)).split(sep) should be abspath(start).split(sep)
* I'm not sure it's worth checking that there's a path. I
noticed that abspath() didn't have a similar check. I
didn't look for other places, but doubt there is much error
checking since a reasonable exception should be raised.
* The for loop is a lot to swallow. It would be easier to
read if you used a local variable for the min_len IMO.
* I'd like to see a test case for: relpath("a", "a")
* I'd like to see test cases in ntpath for paths with a
drive letter
* I'd like to see test cases for the exceptions raised in
ntpath
Another note that isn't a big deal. It's good that you made
a single patch file. I think most other developers prefer
the patch to start one level up. ie, don't start in Lib/,
include it in the patch. I certainly prefer it this way so
I don't have to cd much. It just makes development a little
easier.
You can attach new versions to this tracker item, but try to
remember to add a description that says version # so no one
reviews an older version. |
|
Date |
User |
Action |
Args |
2007-08-23 15:44:22 | admin | link | issue1339796 messages |
2007-08-23 15:44:22 | admin | create | |
|