Message2557
This is some really nice work. I love code that keeps from repeating itself this well while remaining clear. I do have a few concerns related to internal Jython usage as you guessed might happen.
I'm not sure that you actually need the getIterator method. I think you can just use the PyObject returned by pyObj.__iter__() in there and take a PyObject in ItertoolsIterator.nextElement and call __iternext__ on it instead of next(). Is there another reason you're wrapping?
Instead of your toInt method, you can use Py.py2int. I realize that's going to break test_izip since it's expecting ValueErrors for most things, but I think that's actually a bug in the test case. A ValueError is for an illegal value of the right type, whereas a TypeError as thrown by Py.py2int is for the wrong type being passed in entirely as is the case with several of the ValueError checks in test_izip. It's not a big deal to change the test since we're already going to have to for the existing CPython specific failure.
Would it make more sense to use a Java ArrayList in cycle as saved rather than a PyList? A PyList has a bit of overhead in it compared to a regular Java List and since it isn't exposed to Python code there's no reason to use a PyList.
Do the doctests run for you? They still don't for me with the operator patch applied. I get "AttributeError: class 'org.python.modules.operator' has no attribute '__module__'" though that probably isn't your fault.
I hope the wait for a review hasn't put you off working on this. If you upload another patch with the issues I mentioned addressed, I'll happily apply it. I don't think we need to fix doctest to get it in, so just the itertools issues are enough for me. |
|
Date |
User |
Action |
Args |
2008-02-20 17:18:36 | admin | link | issue1608656 messages |
2008-02-20 17:18:36 | admin | create | |
|