From: SourceForge.net <no...@so...> - 2007-10-26 08:37:26
|
Patches item #1804242, was opened at 2007-09-28 19:31 Message generated for change (Comment added) made by mehendran You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=312867&aid=1804242&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: Open Resolution: None Priority: 5 Private: No Submitted By: Mehendran (mehendran) Assigned to: Nobody/Anonymous (nobody) Summary: Collections module Initial Comment: 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 ---------------------------------------------------------------------- >Comment By: Mehendran (mehendran) Date: 2007-10-26 14:07 Message: Logged In: YES user_id=1831942 Originator: YES 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 ---------------------------------------------------------------------- Comment By: Mehendran (mehendran) Date: 2007-10-19 12:06 Message: Logged In: YES user_id=1831942 Originator: YES I will come with the updated patch soon ---------------------------------------------------------------------- Comment By: Charles Groves (cgroves) Date: 2007-10-15 08:52 Message: Logged In: YES user_id=1174327 Originator: NO 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. ---------------------------------------------------------------------- Comment By: Mehendran (mehendran) Date: 2007-10-10 18:20 Message: Logged In: YES user_id=1831942 Originator: YES I have modified the code as per the comments and attached herewith. File Added: new_collections.diff ---------------------------------------------------------------------- Comment By: Charles Groves (cgroves) Date: 2007-10-06 10:33 Message: Logged In: YES user_id=1174327 Originator: NO 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. ---------------------------------------------------------------------- Comment By: Mehendran (mehendran) Date: 2007-09-28 19:44 Message: Logged In: YES user_id=1831942 Originator: YES Testcases which fails are commented and removed from the regrtest run in the test_deque.py. ---------------------------------------------------------------------- Comment By: Mehendran (mehendran) Date: 2007-09-28 19:42 Message: Logged In: YES user_id=1831942 Originator: YES 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. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=312867&aid=1804242&group_id=12867 |