Several points here:
>writing unit tests for functionality that's this closely tied to filesystem
>and behaves differently on different platforms is not trivial
Aha. You see the inherent difficulties :->
Check the Python source distribution.
Python 2.2.3 and later versions include ...\Lib\test\test_ntpath.py, test_posixpath.py and test_macpath.py, with at least reasonable tests for join, normpath , split, splitunc, splitdrive, isabs, abspath and commonprefix. test_os.path contains additional tests, for temp files and stat, though we know the latter's rather pointless right now. Those are more involved too, based on unittest. The others, test_*path.py, just 'from test_support import verbose, TestFailed', and neither really matters -- verbose is just =0 or =1 tested at the end if a successful run should be silent or say "No errors"; TestFailed is just a convenient exception used in test_*path.py to raise an exception at the end if an error's encountered.
In short, test_*path.py should be trivially liftable from CPython, with trivial (irrelevant) changes. That at least buys us a significant amount of testing 'for free'. I'll look thru the rest of CPython's test cases, see if there's more Jython could benefit from.
>It looks like _tostr was added...don't think we really want to restrict things
>to basestring only, but actually want to ensure that None doesn't get
>passed to java.io.File.
Sigh. With the URL, yes, I concur. It's a shame _tostr() was injected.
But one wonders - why?
Why even check?
CPython doesn't.
ntpath.py implements dirname() as just return split(p)[0], and you get the usual exception
Python 2.3.5 (#62, Feb 8 2005, 16:23:02) [MSC v.1200 32 bit (Intel)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import os.path
>>> os.path.dirname(None)
Traceback (most recent call last):
File "<stdin>", line 1, in ?
File "E:\Python23\lib\ntpath.py", line 206, in dirname
return split(p)[0]
File "E:\Python23\lib\ntpath.py", line 163, in split
d, p = splitdrive(p)
File "E:\Python23\lib\ntpath.py", line 118, in splitdrive
if p[1:2] == ':':
TypeError: unsubscriptable object
>>>
However, CPython is rather inconsistent. For instance, commonprefix()'s first line is
if not m: return ''
aka None testing. However, that appeasr to be unique -- nothing else in the *path.py's seem to explicitly test; some coincidentally do, e.g. exists() does an os.stat() in a try block returning 0 if anything's raised, but that seems more coincidental than intentional.
I'll submit a patch to lift the _tostr() restriction, but I can see 2 options:
* Change _tostr() to _nonnull(), replacing if isinstance(s,basestring) with if s!=None
* Remove _tostr(), i.e. revert r2009 and mark 495602 as 'Work as Designed'
Personally, I prefer the latter. It's more 'standard' <sic>, it's less work, and special-casing one small piece of the library for invalid None parameters seems, overall, more harm than good. Sure, NPE's are no fun - but consistency is a powerful force. After all, if we didn't like consistency so, we'd just use Perl ;-)
There's a 3rd option, that may be desirable
* Change _tostr() to _nonnull(), but make the test if s!=None or not java.lang.System.getProperty('jython.debug.NPE')
Use a config option (jython.debug.NPE=<boolean>, default=False) to enable 'pretty NPE exceptions'. In this case, _nonnull() could be a broadly used function, anyplace None is not valid it could be trapped and kick a pretty exception instead of what Jython routinely kicks now
[F:\jython2.2b2]jython.bat -c "import java.io.File; java.io.File(None)"
Traceback (innermost last):
File "<string>", line 1, in ?
at java.io.File.<init>(Unknown Source)
at sun.reflect.GeneratedConstructorAccessor1.newInstance(Unknown Source)
at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(Unknown Source)
at java.lang.reflect.Constructor.newInstance(Unknown Source)
java.lang.NullPointerException: java.lang.NullPointerException
Personally, I'm in favor of the 1st option, ripping out _tostr() and just letting the runtime do its thing, but if people are _really_ passionate about preventing the user from tripping an NPE - despite their explicit wishes - then at least make it an optional debugging feature and not enforced baseline.
I still think that's wrong. It's too Java-ish and not truly Pythonic <g>, and the extent of inconsistency is going to be a problem for a long time to come (unless a lot of effort's put in to large parts of the library, which doesn't seem likely or, frankly, justifiable).
What's your thoughts on the underlying-philosophical question?
I can make a patch for any of the above, but the underlying design philosophy is more significant and broader impact. How do you think Jython should proceed?
>I think it's too late in the release process for 2.2 to make the change
I suspected. I figured it'd be fine if there was a b3, but if release is imminent...sigh.
Ah well.
>Please continue working on it and submit a patch though.
>I'll happily put it in the 2.3 branch.
Then I'll happily proceed :-> |
