Message2557

Author cgroves
Recipients
Date 2007-02-11.06:42:40
SpamBayes Score
Marked as misclassified
Message-id
In-reply-to
Content
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.
History
Date User Action Args
2008-02-20 17:18:36adminlinkissue1608656 messages
2008-02-20 17:18:36admincreate