Issue1817565

This patch provides an implementation for the java.util.Map interface on the PyDictionary object. This allows passing a dictionary created in Jython to java methods.  The patch provides associated wrapper classes so that the dictionary can be manipulated in java, and the changes will be reflected in Jython.

This patch applies to Trunk, It also looks like it will apply to 2.2.1 but there's a little conflict around the imports.

There patch adds two additional files:

src/org/python/core/Dict2JavaTest - for unit tests in java code (had to do this because some tests can't be run in jython).

Lib/test/test_dict2java.py - Jython unit testing script.

 

Added set functionality to values and keySet methods so that the collection returned is backed by PyDictionary.
File Added: dict2java.patch

The internal view collections of PyMapEntrySet and PyMapKeyValSet are both expecting to extend PyMapSet, but there is no such class in the patch.  I'm guessing PyMapCollection was supposed to be renamed to PyMapSet?  

Those two classes also need to implement Set to be acceptable as return values from keySet and entrySet, so perhaps PyMapSet was supposed to implement Set?  From a cursory examining, it seems like PyMapCollection/Set could be greatly simplified by extending AbstractSet rather than implementing the whole interface on its own.  Many of the implemented methods could just go away.

The iterator implementation is identical between the two Set classes, so it should be shared in the super class.

Unless there's a name conflict, a Java class should be imported with an import statement and just its short name should be used in the code.  I see the full use of java.util.Iterator.

I'd move the values method down into the Map implementation section now that it belongs there.

It'd be better to move Dict2JavaTest into Lib/src/javatests so it's separated out with the tests.

Tests using unittest.TestCases are much preferred to a list of asserts as  in test_dict2java.py.  With a list of asserts, the first test failure means the rest of the tests aren't run which makes it harder to track down errors.

I haven't run the tests yet since this doesn't compile, but if you'll fix the first few errors I can run the tests and really evaluate the patch.  The suboptimal use of asserts isn't a deal breaker, it'd just be nice if it were fixed.

File Added: dict2java_v2.patch

File Added: dict2java_v2.patch

> The internal view collections of PyMapEntrySet and
> PyMapKeyValSet are both expecting to extend PyMapSet, but there
> is no such class in the patch.  I'm guessing PyMapCollection
> was supposed to be renamed to PyMapSet?

Sorry, I goofed.  There was a stale class on my side that allowed
it to still compile.  Fixed.

> From a cursory examining, it seems like PyMapCollection/Set
> could be greatly simplified by extending AbstractSet rather
> than implementing the whole interface on its own.  Many of the
> implemented methods could just go away.

Not Really, the problem is that (Which is true in general)
objects retrieved from the dictionary in java code must pass
through __tojava__, and java object being passed to the
dictionary must use Py.java2py which utilizes the plugable
PyObjectAdapter system. Using AbstractSet would eliminate size()
and isEmpty(), but I think that's about it.  Allot of the code
implements the obscure java.util.Map#entrySet method which will
probably never get used, but I added it for completeness.

> The iterator implementation is identical between the two Set
> classes, so it should be shared in the super class.

Done

> Unless there's a name conflict, a Java class should be imported
> with an import statement and just its short name should be used
> in the code.  I see the full use of java.util.Iterator.

Done

> I'd move the values method down into the Map implementation
> section now that it belongs there.

Done

> It'd be better to move Dict2JavaTest into Lib/src/javatests so
> it's separated out with the tests.

I found a Lib/test/javatests which I moved Dict2JavaTest to.
Done

> Tests using unittest.TestCases are much preferred to a list of
> asserts as in test_dict2java.py.  With a list of asserts, the
> first test failure means the rest of the tests aren't run which
> makes it harder to track down errors.

Done

> > From a cursory examining, it seems like PyMapCollection/Set
> > could be greatly simplified by extending AbstractSet rather
> > than implementing the whole interface on its own.  Many of the
> > implemented methods could just go away.
> 
> Not Really, the problem is that (Which is true in general)
> objects retrieved from the dictionary in java code must pass
> through __tojava__, and java object being passed to the
> dictionary must use Py.java2py which utilizes the plugable
> PyObjectAdapter system. Using AbstractSet would eliminate size()
> and isEmpty(), but I think that's about it.  Allot of the code
> implements the obscure java.util.Map#entrySet method which will
> probably never get used, but I added it for completeness.

Since most of AbstractSet operates on the Iterator returned by its concrete implementation, and PyMapSet has to implement iterator, the methods on AbstractSet will go through that iterator.  I've attached a new version of your patch that uses AbstractSet and passes your tests.  It eliminates 9 methods: hashCode, equals, retainAll, removeAll, containsAll, add, addAll, toArray and isEmpty.  Am I missing something?

File Added: dict2java_groves.patch

> Since most of AbstractSet operates on the Iterator returned by its
> concrete implementation, and PyMapSet has to implement iterator, the
> methods on AbstractSet will go through that iterator.  I've attached
> a new version of your patch that uses AbstractSet and passes your
> tests.  It eliminates 9 methods: hashCode, equals, retainAll,
> removeAll, containsAll, add, addAll, toArray and isEmpty.  Am I
> missing something?

Yea, that looks good, I didn't realize that the AbstractSet explicitly
used the Iterator for the interface, as opposed to an implementation
detail subject to possible change.  Could probably remove size() and
clear() also.

It would be possible to remove the size and clear operations, but they're going to be so much faster on the actual collection than with an iterator on top of them.

As such, I committed the AbstractSet version of the patch in r3802.  Thanks!