Hi Thomas,
Here are comments inline on your patch, quoting just the relevant fragments.
Thank you !
Andi..
Index: python/collections.py
===================================================================
--- python/collections.py (revision 1292224)
+++ python/collections.py (working copy)
def remove(self, obj):
try:
self._set.remove(obj)
@@ -104,7 +104,7 @@
def retainAll(self, collection):
result = False
for obj in list(self._set):
- if obj not in c:
+ if obj not in collection:
self._set.remove(obj)
result = True
return result
Whoops. Thank you. Integrated.
+class JavaListIterator(PythonListIterator):
+ """
+ This class implements java.util.ListIterator for a Python list instance it
wraps.
+ (simple bidirectional iterator)
+ """
+ def __init__(self, _lst, index=0):
+ super(JavaListIterator, self).__init__()
+ self._lst = _lst
+ # TODO: raise JavaError for IndexOutOfBoundsException!?
+ assert (index>=0 and index<len(_lst)), "index is not out of range"
+ self.index = index
I'd not check index here and raise StopIteration later as needed.
That way, if _lst changes and index "becomes" valid, things still work.
Or conversely, if _lst changes and index becomes invalid, you're not depending
on this check.
+ def next(self):
+ try:
+ result = self._lst[self.index]
+ self.index += 1
+ except IndexError:
+ # TODO: raise JavaError for NoSuchElementException!?
+ raise StopIteration
+ return result
Why not just check for self.index to be in range here and raise StopIteration
instead of relying in the exception ? It's cheaper and that's how you do it
below, it's good to be consistent.
+ def previous(self):
+ self.index -= 1
+ if self.index < 0:
+ # TODO: raise JavaError for NoSuchElementException!?
+ raise StopIteration
+ return self.collection[self.index]
According to the javadocs, this method is supposed to throw
NoSuchElementException. Raising StopIteration is not going to do the trick.
Same comment on the previous method too.
+ def hasPrevious(self):
+ return self.index>0
+
+ def hasNext(self):
+ return self.index<len(self._lst)
+
+ def nextIndex(self):
+ return min(self.index,len(self._lst))
+
+ def previousIndex(self):
+ return max(self.index,-1)
Please, be consistent and use spaces between operators and after commas.
+ def __iter__(self):
+ return self
Why not also implement remove() and set() ?
+
+class JavaList(PythonList):
+ """
+ This class implements java.util.List around a Python list instance it
wraps.
+ """
+
+ def __init__(self, _lst):
+ super(JavaList, self).__init__()
+ self._lst = _lst
+
+ def __contains__(self, obj):
+ return obj in self._lst
+
+ def __len__(self):
+ return len(self._lst)
+
+ def __iter__(self):
+ return iter(self._lst)
+
+ def add(self, index, obj):
+ self._lst.insert(index, obj)
+
+ def addAll(self, collection):
+ size = len(self._lst)
+ self._lst.extend(collection)
+ return len(self._lst) > size
+
+ def addAll(self, index, collection):
+ size = len(self._lst)
+ self._lst[index:index]=collection
+ return len(self._lst) > size
+
+ def clear(self):
+ del self._lst
+ self._lst = []
Why not clear the list in place with del self._lst[:] ?
Changing the _lst reference is going to trip over users who assume that _lst
is always what they put in to begin with.
+ def contains(self, obj):
+ return obj in self._lst
+
+ def containsAll(self, collection):
+ for obj in collection:
+ if obj not in self._lst:
+ return False
+ return True
+
+ def equals(self, collection):
+ if type(self) is type(collection):
+ return self._lst == collection._lst
+ return False
+
+ def get(index):
+ return self._lst[index]
What if index is out of range ?
+
+ def isEmpty(self):
+ return len(self._lst) == 0
+
+ def iterator(self):
+ class _iterator(PythonIterator):
+ def __init__(_self):
+ super(_iterator, _self).__init__()
+ _self._iterator = iter(self._lst)
+ def hasNext(_self):
+ if hasattr(_self, '_next'):
+ return True
+ try:
+ _self._next = _self._iterator.next()
+ return True
+ except StopIteration:
+ return False
+ def next(_self):
+ if hasattr(_self, '_next'):
+ next = _self._next
+ del _self._next
+ else:
+ next = _self._iterator.next()
+ return next
+ return _iterator()
+
+ def lastIndexOf(obj):
+ try:
+ return len(self._lst)-1-self._lst[::-1].index(obj)
+ except ValueError:
+ return -1
Please use spaces between operators.
Wouldn't it be more efficient to iterate backwards until the element is found
instead of copying the list (self._lst[::-1]) and iterate forwards ?
+
+ def listIterator(self):
+ return JavaListIterator(self._lst)
+
+ def listIterator(self, index):
+ return JavaListIterator(self._lst, index)
+
+ def remove(self, obj_or_index):
+ if type(obj_or_index) is type(1):
+ return removeAt(int(obj_or_index))
+ return removeElement(obj_or_index)
It's better to do this at the Java level.
Declare differently named native methods for each overload of remove() and
implement remove(int) in Java to call removeInt(int)
and remove(Object) to call removeObject(Object) (or whatever you name them)
+ def removeAt(self, pos):
+ """Removes the element at the specified position in this list
+ """
+ try:
+ el = self._lst[pos]
+ del self._lst[pos]
+ return el
+ except IndexError:
+ # TODO: raise JavaError for IndexOutOfBoundsException!?
+ return None
Why not check the index to be in range instead of try/except ?
+ def removeElement(self, obj):
+ """Removes the first occurrence of the specified element from this
list, if it is present
+ """
+ try:
+ self._lst.remove(obj)
+ return True
+ except ValueError:
+ return False
+
+ def removeAll(self, collection):
+ result = False
+ for obj in collection:
+ if self.removeElement(obj):
+ result = True
+ return result
+
+ def retainAll(self, collection):
+ result = False
+ for obj in self._lst:
+ if (obj not in collection
+ and self.removeElement(obj)):
+ result = True
+ return result
+
+ def size(self):
+ return len(self._lst)
+
+ def toArray(self):
+ return self._lst
+
+ def subList(fromIndex, toIndex):
+ sublst = self._lst[fromIndex:toIndex]
+ return JavaList(sublst)
The javadoc expects this method to throws IndexOutOfBoundsException instead of
behaving nice like a Python slice.
+ def set(index, obj):
+ try:
+ self._lst[index]=obj
+ except IndexError:
+ raise
+ #TODO raise JavaError for IndexOutOfBoundsException instead?!
Please use spaces between operators.
Index: java/org/apache/pylucene/util/PythonListIterator.java
===================================================================
+public class PythonListIterator extends PythonIterator implements ListIterator
{
+
+ // private long pythonObject;
+
+ public PythonListIterator()
+ {
+ }
+
+ /* defined in super class PythonIterator:
+ public void pythonExtension(long pythonObject)
+ {
+ this.pythonObject = pythonObject;
+ }
+ public long pythonExtension()
+ {
+ return this.pythonObject;
+ }
+ */
If this work, you don't need pythonObject to be protected anymore in the
superclass then ?
+ public native boolean hasPrevious();
+ public native Object previous();
+
+ public native int nextIndex();
+ public native int previousIndex();
+
+ public void set(Object obj) {
+ throw new UnsupportedOperationException();
+ }
What about remove () ?
+ public void add(Object obj) {
+ throw new UnsupportedOperationException();
+ }
Why not support them ?
+
+}
Index: java/org/apache/pylucene/util/PythonIterator.java
===================================================================
--- java/org/apache/pylucene/util/PythonIterator.java (revision 1292224)
+++ java/org/apache/pylucene/util/PythonIterator.java (working copy)
@@ -20,7 +20,7 @@
public class PythonIterator implements Iterator {
- private long pythonObject;
+ protected long pythonObject;
This shouldn't be needed anymore according to the code commented out above.
Index: java/org/apache/pylucene/util/PythonList.java
===================================================================
+ public native boolean add(Object obj);
+ public native void add(int index, Object obj);
+ public native boolean addAll(Collection c);
+ public native boolean addAll(int index, Collection c);
+ public native void clear();
+ public native boolean contains(Object obj);
+ public native boolean containsAll(Collection c);
+ public native boolean equals(Object obj);
+ public native Object get(int index);
+ // public native int hashCode();
+ public native int indexOf(Object obj);
+ public native boolean isEmpty();
+ public native Iterator iterator();
+ public native int lastIndexOf(Object obj);
+ public native ListIterator listIterator();
+ public native ListIterator listIterator(int index);
+ public native Object remove(int index);
+ public native boolean remove(Object obj);
Here you should declare different names for the remove() overloads and have
the remove() overloads invoke each native method accordingly.
For example:
public native removeAt(int index);
public Object remove(int index)
{
return removeAt(index);
}
public native boolean removeObject(Object obj);
public boolean remove(Object obj)
{
return removeObject(obj);
}
