Issue1608656

A patch for the itertools module in 2.3. The test coverage for the module is quite extensive, so I feel pretty confident that it works "as designed". One test fails, but this is a test of an implementation detail i CPython (reusing tuples). This is my first module port, so some things may not be "optimal" when it comes to the usage of internal Jython APIs. Feedback is very welcome. Oh, and I've actually never used svn before, so my file properties may be a bit off... 

'Henrik

Wow, that's quite a bit of work. Dumb question #1: are the changes to modules/operator.java at the end of the patch somehow related to itertools, or are they part of some other set of fixes?

- kz

The itertools tests used the pow function in operator. But it's not directly related to itertools. 

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.

This is exactly the kind of feedback I was looking for. Thanks!

Hmmm... it's been a while so I'm not 100% sure, but I think I wanted to wrap it to be able to use PyIterator's next() method logic for throwing the stored exception... might be overkill, I'll have a look at it.

You're right about toInt vs Py.py2int. I stared myself blind at getting the tests to pass :-) 

I also agree about the PyList in cycle. It was quite handy to be able to get a PyIterator from it that easily. But as you say, it has some unnecessary overhead to it in this case. I'll make sure to fix that.

I'll get on it as soon as I get my new laptop (I recently switched employer and haven't recieved one yet). Might take a week or two though :-( 
I don't remember seeing any doctest failures but now I'm not sure... I'll get back to you on that one.

I think you'll still get the PyIterator next element handling by continuing to use your ItertoolsIterator class.  The underlying iterator passed into ItertoolsIterator.nextElement will throw StopIteration when it's done and you already handle that.

If using List vs. PyList significantly complicates the code, I wouldn't worry about it.  The overhead isn't worth adding much complexity.

I can't verify it right now, but I think I need to wrap the iterable when it isn't an iterable per say... Have a look at classes G through S in test_itertools.py for examples. I'm pretty sure some of those won't work without wrapping.

I'll have a closer look later.

You were right about the wrapping (of course). It's totally unnecessary. I wasn't sure I liked the idea of using PyObject everywhere at first though, but when I thought about it from a jython perspective instead of a java perspective it makes sense :-)

File Added: itertools.patch

Two new patches uploaded. One for the module (with all suggested changes) and one for the test.
File Added: test_itertools.patch

Looks like a winner to me.  Committed in r3106-3108.  I modified the patch to remove the toInt method, and to make the test run under CPython even with the Jython modifications.

I am still getting an AttributeError when it tries to run the doctest:  

Traceback (innermost last):
  File "dist/Lib/test/test_itertools.py", line 650, in ?
  File "dist/Lib/test/test_itertools.py", line 647, in test_main
  File "/Users/groves/dev/jython/2.3/dist/Lib/test/test_support.py", line 288, in run_doctest
  File "/Users/groves/dev/jython/2.3/dist/Lib/doctest.py", line 1148, in testmod
  File "/Users/groves/dev/jython/2.3/dist/Lib/doctest.py", line 906, in rundict
  File "/Users/groves/dev/jython/2.3/dist/Lib/doctest.py", line 589, in _from_module
AttributeError: class 'org.python.modules.operator' has no attribute '__module__'

Do you get that?  

I still committed it since I think that's probably a general doctest failure.  Now that it's in, would you mind sending in a Python Contributor form from http://www.python.org/psf/contrib/contrib-form/?

DOH! I uploaded the wrong patch... I removed the toInt method and cleaned up the imports, but I obviously created the patch before that. Thanks for cleaning it up. 

I do get the same doctest failures. But as you say it's probably a general doctest failure. I can have a quick look at it next week.

OK, I'll make sure to send in the form.