Message2750
This is a decent start towards reversed, but there are a several problems with the patch.
First, as a general rule, a patch should just be a single diff file. If you svn add PySequenceReverseIter.java and test_reversed.py in your local checkout, and then run 'svn diff > reversed_builtin.diff' at the base of your checkout you'll get a single file containing all of your changes. This is a single operation for someone else to apply it to check it out. By zipping it up you've made it a several step operation.
Secondly, your test_reversed.py relies on someone to manually verify its output. Any new test for Python functionality in Jython should use the unittest module and be placed in Lib/test/ so it can be run by regrtest.py. If you do that it'll be run by regrtest and checked along with everything else. Look at some of the files in Lib/test that use unittest for examples of how it should be done. I'd also check for existing tests for reversed in the CPython 2.4 or 2.5 Lib/test directory. I'd be surprised if there aren't some already and you should make sure all of those work and include them in your patch.
Now, on to the actual implementation. I wouldn't call the class PyReversedSequenceIterator; PyReversedIterator will do since it doesn't require a Sequence. In the same vein, you should remove the instanceof PySequence check in __builtin__. The pep just states that an object passed to reversed needs __getitem__ and __len__ methods, so just check for and use those methods.
There's no need to check for StopIteration or catch PyException at all in your __iternext__. You're not using the iteration functions so StopIteration has nothing to do with with your calls.
Using negative indexing probably isn't a good idea: supporting it isn't a requirement of a sequence like object. Instead I'd get the last index with __len__, and start grabbing items with __getitem__ from it counting down. |
|
Date |
User |
Action |
Args |
2008-02-20 17:18:44 | admin | link | issue1761139 messages |
2008-02-20 17:18:44 | admin | create | |
|