[Jython-bugs] [ jython-Patches-1761139 ] Added builtin function [PEP 322: Reverse Iteration]

SourceForge Headquarters 225 Broadway Suite 1600 San Diego, CA 92101 +1 (858) 454-5900

Patches item #1761139, was opened at 2007-07-26 08:40
Message generated for change (Comment added) made by cgroves
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=312867&aid=1761139&group_id=12867

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: None
Group: None
>Status: Closed
>Resolution: Accepted
Priority: 5
Private: No
Submitted By: Mehendran (mehendran)
Assigned to: Nobody/Anonymous (nobody)
Summary: Added builtin function [PEP 322: Reverse Iteration]

Initial Comment:
Implementation of 'reversed(seq)' built-in function.
Attached is the zip file. Please unzip it and follow
the ReadMe.txt

----------------------------------------------------------------------

>Comment By: Charles Groves (cgroves)
Date: 2007-07-29 22:27

Message:
Logged In: YES 
user_id=1174327
Originator: NO

There were still a couple problems in the latest patch, but they were
small enough that I went ahead and fixed them and committed this in r3365
on the 2.3 branch.

First, don't include commented out printouts or any stuff like that that
shouldn't be in the final committed code in the patch.  It should be in a
state where I can apply the patch and commit it without modification.

An object being an instance of PyDictionary doesn't make it a dict in the
same way that an object being a PySequence doesn't make it a sequence. 
Instead of checking instanceof, I changed it to look for the keys attr like
the code does in CPython.

We're trying to increase the javadoc coverage on publicly exposed methdods
and classes in Jython, so anything that's at that level should have some
javadocs.

I deleted the completely commented out test from test_enumerate.  Is
adding a __reversed__ method to classes something from 2.6?  I didn't see
anything about it in the 2.5 source I looked at.  In any case, if something
is going to need additional features later, I'd recommend filing a bug for
it rather than burying it in commented out source code.  No one is going to
go through the source looking for things that are missing, but a bug won't
get lost like that.

----------------------------------------------------------------------

Comment By: Mehendran (mehendran)
Date: 2007-07-27 10:01

Message:
Logged In: YES 
user_id=1831942
Originator: YES

I've modified code according to cgroves. 
And I've attached a patch for that.
Kindly get back to me if there is any problem.
File Added: reversed_builtin.diff

----------------------------------------------------------------------

Comment By: Mehendran (mehendran)
Date: 2007-07-27 03:53

Message:
Logged In: YES 
user_id=1831942
Originator: YES

Thanks, you clarified me. 
I will get get with modifications.

----------------------------------------------------------------------

Comment By: Charles Groves (cgroves)
Date: 2007-07-27 01:37

Message:
Logged In: YES 
user_id=1174327
Originator: NO

Right, that's the same thing I was reading in the pep.  It's saying it
should work on anything that has __getitem__ and __len__ which doesn't
correspond to PySequence in Jython.  PySequence is just the baseclass for
builtin sequence types like PyList and PyTuple.  Any PyObject can implement
those methods.

PySequence_Check in CPython is the following:

int
PySequence_Check(PyObject *s)
{
	if (s && PyInstance_Check(s))
		return PyObject_HasAttrString(s, "__getitem__");
	if (PyObject_IsInstance(s, (PyObject *)&PyDict_Type))
		return 0;
	return s != NULL && s->ob_type->tp_as_sequence &&
		s->ob_type->tp_as_sequence->sq_item != NULL;
}

Essentially it returns true if the object is an instance of a user-defined
class with a __getitem__ attribute, or if it isn't a subclass of dict and
it is a builtin sequence type.

----------------------------------------------------------------------

Comment By: Mehendran (mehendran)
Date: 2007-07-27 01:22

Message:
Logged In: YES 
user_id=1831942
Originator: YES

Thanks for your comments.

I have seen sentences in the PEP like, 

"This proposal is to add a builtin function to support 
reverse iteration over sequences."

"Add a builtin function called reversed() that makes a 
reverse iterator over sequence objects that support 
__getitem__() and __len__()"

And also, in cpython implementation they verified like
in the following

+ 	if (!PySequence_Check(seq)) {

+ 		PyErr_SetString(PyExc_TypeError,

+ 		"argument to reversed() must be a sequence");

+ 		return NULL;

+ 

+ 	}

I maybe interpreted "Sequences" wrongly. 

Kindly verify once and make me understand.

http://www.python.org/doc/2.4.3/whatsnew/node7.html
http://www.python.org/dev/peps/pep-0322/

Thanks,
Mehendran

----------------------------------------------------------------------

Comment By: Charles Groves (cgroves)
Date: 2007-07-26 23:12

Message:
Logged In: YES 
user_id=1174327
Originator: NO

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.

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=312867&aid=1761139&group_id=12867