Issue1804242

I have attached the patch for collections module. This is  my attempt to add a new module in jython. Kindly review this patch and give me your valuable suggestions/comments. 

collections module contains two data structures currently. And test cases test_deque.py and test_defaultdict.py are to test each of them respectively.

Please let me know if you have any queries...

-Mehendran T

Actually Deque is not fully functional. For that, I need support for pickling and weakref.
Some of the test cases failed due to the noexistence of "seq_tests" and "gc" modules.
But for pickling and weakreference, i need support though implementation is done. 
 

Testcases which fails are commented and removed from the regrtest run 
in the test_deque.py.

Some basic formatting concerns:
  - A space between a parenthesis and a brace.
  - An empty line between all method declarations.
  - Braces on all loops and if/else statements.
  - I'm glad to see you included javadoc, but don't add @param or @return if
    there aren't params or returns or if you're not going to clarify on them.

One basic Jython concern:
  - Never override __getitem__, only __finditem__.

Some general coding concerns:
  - Don't copy code.  It looks like you copied dict_init into dictionary_init
    in defaultdict.  You should've made dict_init callable from there.
  - Don't prepend _ to method names.  We have private in Java.
  - Don't add constructors that don't have an immediate use(in PyDefaultdict).

Overall this seems like a good start, so if you'll fix these little things, I'll actually examine the implementation.

I have modified the code as per the comments and attached herewith.
File Added: new_collections.diff

Was there a reason only dicts of the same type were comparable before?  It's something that appeared before my time, so I'm not sure what the reasoning is behind the current behavior, but I doubt it's a random choice.  What's the behavior in CPython?  Returning null from __eq__ or -2 from __cmp__, the bevhavior before the patch, indicates it isn't implemented for that type.  If that's the case, the __eq__ or __cmp__ from the other type is called, so this could be implemented in PyDefaultdict instead without modifying PyDictionary.  

Similarly, what's the thinking behind removing the getArray call on PyTuple in PyException.  It's another thing that has existed forever, so I don't know its provenance, but it seems like some pretty specific behavior, and the change is quite different.  Is there no code that uses the existing tuple behavior?  Why is the change needed?

In PyDefaultDict, I wouldn't expose the default_factory as public since you have getter and setter methods for it.  Just make it private so all access is known to go through one place.  

Why does the __missing__ implementation use eval to call the factory?  default_factory.__call__() should have the same effect, and I'm not even sure what would happen if the factory method weren't available in the current scope.  Also, seeing because written out as "bcoz" makes me shudder :)

There isn't a __copy__ method on PyObject, so there's no need to add it to PyDefaultDict.  Your exposing it as copy in the template should be enough.

I'll save looking at deque for later.

I will come with the updated patch soon

I have modified the code according to your comments. 

1) In CPython, dict and subtypes of dict can be comparable. So I need to change
   in PyDictionary. There were two implementations for __eq__ and __cmp__.
   I made it one. The __eq__ and __ne__ will be using __cmp__ function.

2) There is no need of change in PyException. I reverted it to old code.

3) default_factory's access is private now.

4) In __missing__ method, default_factory.__call__() is used instead of 
   eval function.

5) I removed __copy__ function.

File Added: new_collections_one.diff

__copy__ is still on PyDefaultDict and PyException is
still modified.

In PyDeque, curly braces on all blocks.

Instead of iterating over a sequence with explicit calls
to data.__getitem__ in deque_init, you could just call
deque_extend with a PySequenceIter wrapping data.

You shouldn't need to check for __iter__ returning null in
deque_extend.  Was it returning null somewhere?

Use Py.One instead of new PyInteger(1)

Rather than rightlink and leftlink, I'd just call the
fields on Node left and right.

Those are some pretty small complaints though.  It looks like this is just about ready to go.

My doubts and Charlie's answers:
================================
> PyDefaultDict :
> -----------
> CPython has two functions for doing shallow copy.
> 1) copy  2) __copy__
> That's why I too have added two functions. But I removed
>  the class specific __copy__ function because that is not in
> PyObject and we don't need to override.  Do you want me to
> remove copy function?

As far as I can tell, we don't have __copy__ anywhere else in Jython.
Why does PyDefaultdictionary need it?
>
> I didn't change in PyException.java file. But I have added one
> more KeyError function with parameter type PyObject. It is
> needed to throw from __missing__ function of PyDefaultDict.
> In CPython, the key itself is printed as error message. So I
> followed the same. If I didn't add this, I would get errors
> while running test cases.

Is the string value of the key error used, or the key itself?  If it's
the former, just toString the key and pass it through.  No need to
modify PyException.

> PyDeque:
> -------
> I have got confused from the comment
> "In PyDeque, curly braces on all blocks" because you asked me
> to add braces for all blocks when you reviewed defaultdict.
> Here, I interpreted like you want me to remove unnecessary
> curly braces. Please clarify me.

I'm asking for you to finish fixing all of the places without curly
braces on blocks.  I found at least one place(in PyDeque's init I
think) where there were blocks without curly braces.

1) PyDeque also has __copy__ function. Should I remove that too? 
   If I remove that function, I will get errors in test cases.
   So Should I modify the test case file? 

2) When are we going to introduce the __copy__ function in jython?
   It is because CPython supports that. Any .py file which uses this
   __copy__ func will not work on the jython, isn't it?

3) When I tried with passing key's string value, some of the test 
   cases are getting failed. I revisited the CPython code. It
   seems like key itself is passed to be printed.

4) I couldn't find the place where I missed braces. I will be happy if
   you show me where it is.

Please clarify me.

1) I've just looked through the code for the copy module, and finally understand what's going on with __copy__.  __copy__ is called by copy whenever it finds a type it doesn't have a predefined mapping for, so PyDeque and PyDefaultDict will both need it since they have specific copy behavior they want to use.  We haven't needed it anywhere else in Jython because copy handles builtin types explicitly.  So leaving __copy__ on PyDeque and PyDefaultDict is fine and good.

2) From 1, we already support __copy__, it just wasn't used in the core Java code up until now.

3) Looking at the test code in test_defaultdict, passing in the object as a value in a key error seems reasonable.

4) The only seeing one place, in PyDeque.deque_init, so that wasn't too bad.  I just noticed that one and assumed you'd only fixed it in PyDefaultDict.

The only remaining problem is that your equality changes are causing test_richcmp to fail.  DictTests.test_dicts fails for me because equality testing now uses methods other than __eq__ and __ne__ for testing.  If you can fix that, I'll apply the patch.

Thanks a lot. I have done the changes needed. Testcases are 
running properly. I hope it is complete to go.



File Added: new_collections_v2.diff

test_cfgparser still fails on its own and when I run regrtest with the new patch.  Are you not seeing that failure?

I have done regrtest and the problem is solved.
 
File Added: new_collections_v3.diff

Would you mind summarizing the changes you've made since the last patch so I know what to look at?  Or if you could reattach the previous version, I could diff against that.  There's no reason to delete them if you're naming them with a version in them like you are.

I have attached the old patch. I have changed  
PyException.java and exceptions.java. This
is for handling KeyError specifically as CPython.
Thanks.
File Added: new_collections_v2.diff

Applied in r3916.  Thanks!