Author nnorwitz
Date 2005-10-28.06:21:50
SpamBayes Score
Marked as misclassified
Logged In: YES 

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

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

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:22adminlinkissue1339796 messages
2007-08-23 15:44:22admincreate