From: SourceForge.net <no...@so...> - 2007-09-28 14:01:39
|
Patches item #1804242, was opened at 2007-09-28 19:31 Message generated for change (Tracker Item Submitted) made by Item Submitter 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 ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=312867&aid=1804242&group_id=12867 |
From: SourceForge.net <no...@so...> - 2007-09-28 14:04:21
|
Patches item #1804242, was opened at 2007-09-28 19:31 Message generated for change (Settings changed) 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: 9 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 ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=312867&aid=1804242&group_id=12867 |
From: SourceForge.net <no...@so...> - 2007-09-28 14:12:07
|
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: 9 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-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 |
From: SourceForge.net <no...@so...> - 2007-09-28 14:14:41
|
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: 9 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-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 |
From: SourceForge.net <no...@so...> - 2007-09-30 01:28:44
|
Patches item #1804242, was opened at 2007-09-28 09:01 Message generated for change (Settings changed) made by cgroves 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-09-28 09:14 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 09:12 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 |
From: SourceForge.net <no...@so...> - 2007-10-06 05:03:21
|
Patches item #1804242, was opened at 2007-09-28 09:01 Message generated for change (Comment added) made by cgroves 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: Charles Groves (cgroves) Date: 2007-10-06 00:03 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 09:14 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 09:12 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 |
From: SourceForge.net <no...@so...> - 2007-10-10 12:50:16
|
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-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 |
From: SourceForge.net <no...@so...> - 2007-10-15 03:22:45
|
Patches item #1804242, was opened at 2007-09-28 09:01 Message generated for change (Comment added) made by cgroves 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: Charles Groves (cgroves) Date: 2007-10-14 22:22 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 07:50 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 00:03 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 09:14 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 09:12 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 |
From: SourceForge.net <no...@so...> - 2007-10-19 06:36:50
|
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-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 |
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 |
From: SourceForge.net <no...@so...> - 2007-11-12 05:01:15
|
Patches item #1804242, was opened at 2007-09-28 09:01 Message generated for change (Comment added) made by cgroves 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: Charles Groves (cgroves) Date: 2007-11-12 00:01 Message: Logged In: YES user_id=1174327 Originator: NO __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. ---------------------------------------------------------------------- Comment By: Mehendran (mehendran) Date: 2007-10-26 03:37 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 01:36 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-14 22:22 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 07:50 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 00:03 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 09:14 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 09:12 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 |
From: SourceForge.net <no...@so...> - 2007-11-15 12:36:49
|
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-11-15 18:06 Message: Logged In: YES user_id=1831942 Originator: YES 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. ---------------------------------------------------------------------- Comment By: Charles Groves (cgroves) Date: 2007-11-12 10:31 Message: Logged In: YES user_id=1174327 Originator: NO __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. ---------------------------------------------------------------------- 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 |
From: SourceForge.net <no...@so...> - 2007-11-15 12:54:04
|
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-11-15 18:24 Message: Logged In: YES user_id=1831942 Originator: YES 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. ---------------------------------------------------------------------- Comment By: Mehendran (mehendran) Date: 2007-11-15 18:06 Message: Logged In: YES user_id=1831942 Originator: YES 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. ---------------------------------------------------------------------- Comment By: Charles Groves (cgroves) Date: 2007-11-12 10:31 Message: Logged In: YES user_id=1174327 Originator: NO __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. ---------------------------------------------------------------------- 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 |
From: SourceForge.net <no...@so...> - 2007-11-30 20:34:10
|
Patches item #1804242, was opened at 2007-09-28 09:01 Message generated for change (Comment added) made by cgroves 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: Charles Groves (cgroves) Date: 2007-11-30 15:34 Message: Logged In: YES user_id=1174327 Originator: NO 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. ---------------------------------------------------------------------- Comment By: Mehendran (mehendran) Date: 2007-11-15 07:54 Message: Logged In: YES user_id=1831942 Originator: YES 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. ---------------------------------------------------------------------- Comment By: Mehendran (mehendran) Date: 2007-11-15 07:36 Message: Logged In: YES user_id=1831942 Originator: YES 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. ---------------------------------------------------------------------- Comment By: Charles Groves (cgroves) Date: 2007-11-12 00:01 Message: Logged In: YES user_id=1174327 Originator: NO __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. ---------------------------------------------------------------------- Comment By: Mehendran (mehendran) Date: 2007-10-26 03:37 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 01:36 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-14 22:22 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 07:50 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 00:03 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 09:14 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 09:12 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 |
From: SourceForge.net <no...@so...> - 2007-12-07 14:14:37
|
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: 6 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-12-07 19:44 Message: Logged In: YES user_id=1831942 Originator: YES 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 ---------------------------------------------------------------------- Comment By: Charles Groves (cgroves) Date: 2007-12-01 02:04 Message: Logged In: YES user_id=1174327 Originator: NO 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. ---------------------------------------------------------------------- Comment By: Mehendran (mehendran) Date: 2007-11-15 18:24 Message: Logged In: YES user_id=1831942 Originator: YES 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. ---------------------------------------------------------------------- Comment By: Mehendran (mehendran) Date: 2007-11-15 18:06 Message: Logged In: YES user_id=1831942 Originator: YES 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. ---------------------------------------------------------------------- Comment By: Charles Groves (cgroves) Date: 2007-11-12 10:31 Message: Logged In: YES user_id=1174327 Originator: NO __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. ---------------------------------------------------------------------- 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 |
From: SourceForge.net <no...@so...> - 2007-12-08 23:29:29
|
Patches item #1804242, was opened at 2007-09-28 09:01 Message generated for change (Comment added) made by cgroves 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: Charles Groves (cgroves) Date: 2007-12-08 18:29 Message: Logged In: YES user_id=1174327 Originator: NO test_cfgparser still fails on its own and when I run regrtest with the new patch. Are you not seeing that failure? ---------------------------------------------------------------------- Comment By: Mehendran (mehendran) Date: 2007-12-07 09:14 Message: Logged In: YES user_id=1831942 Originator: YES 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 ---------------------------------------------------------------------- Comment By: Charles Groves (cgroves) Date: 2007-11-30 15:34 Message: Logged In: YES user_id=1174327 Originator: NO 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. ---------------------------------------------------------------------- Comment By: Mehendran (mehendran) Date: 2007-11-15 07:54 Message: Logged In: YES user_id=1831942 Originator: YES 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. ---------------------------------------------------------------------- Comment By: Mehendran (mehendran) Date: 2007-11-15 07:36 Message: Logged In: YES user_id=1831942 Originator: YES 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. ---------------------------------------------------------------------- Comment By: Charles Groves (cgroves) Date: 2007-11-12 00:01 Message: Logged In: YES user_id=1174327 Originator: NO __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. ---------------------------------------------------------------------- Comment By: Mehendran (mehendran) Date: 2007-10-26 03:37 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 01:36 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-14 22:22 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 07:50 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 00:03 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 09:14 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 09:12 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 |
From: SourceForge.net <no...@so...> - 2007-12-20 06:40:32
|
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: 6 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-12-20 12:10 Message: Logged In: YES user_id=1831942 Originator: YES I have done regrtest and the problem is solved. File Added: new_collections_v3.diff ---------------------------------------------------------------------- Comment By: Charles Groves (cgroves) Date: 2007-12-09 04:59 Message: Logged In: YES user_id=1174327 Originator: NO test_cfgparser still fails on its own and when I run regrtest with the new patch. Are you not seeing that failure? ---------------------------------------------------------------------- Comment By: Mehendran (mehendran) Date: 2007-12-07 19:44 Message: Logged In: YES user_id=1831942 Originator: YES 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 ---------------------------------------------------------------------- Comment By: Charles Groves (cgroves) Date: 2007-12-01 02:04 Message: Logged In: YES user_id=1174327 Originator: NO 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. ---------------------------------------------------------------------- Comment By: Mehendran (mehendran) Date: 2007-11-15 18:24 Message: Logged In: YES user_id=1831942 Originator: YES 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. ---------------------------------------------------------------------- Comment By: Mehendran (mehendran) Date: 2007-11-15 18:06 Message: Logged In: YES user_id=1831942 Originator: YES 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. ---------------------------------------------------------------------- Comment By: Charles Groves (cgroves) Date: 2007-11-12 10:31 Message: Logged In: YES user_id=1174327 Originator: NO __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. ---------------------------------------------------------------------- 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 |
From: SourceForge.net <no...@so...> - 2007-12-20 08:55:16
|
Patches item #1804242, was opened at 2007-09-28 09:01 Message generated for change (Comment added) made by cgroves 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: 6 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: Charles Groves (cgroves) Date: 2007-12-20 03:55 Message: Logged In: YES user_id=1174327 Originator: NO 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. ---------------------------------------------------------------------- Comment By: Mehendran (mehendran) Date: 2007-12-20 01:40 Message: Logged In: YES user_id=1831942 Originator: YES I have done regrtest and the problem is solved. File Added: new_collections_v3.diff ---------------------------------------------------------------------- Comment By: Charles Groves (cgroves) Date: 2007-12-08 18:29 Message: Logged In: YES user_id=1174327 Originator: NO test_cfgparser still fails on its own and when I run regrtest with the new patch. Are you not seeing that failure? ---------------------------------------------------------------------- Comment By: Mehendran (mehendran) Date: 2007-12-07 09:14 Message: Logged In: YES user_id=1831942 Originator: YES 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 ---------------------------------------------------------------------- Comment By: Charles Groves (cgroves) Date: 2007-11-30 15:34 Message: Logged In: YES user_id=1174327 Originator: NO 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. ---------------------------------------------------------------------- Comment By: Mehendran (mehendran) Date: 2007-11-15 07:54 Message: Logged In: YES user_id=1831942 Originator: YES 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. ---------------------------------------------------------------------- Comment By: Mehendran (mehendran) Date: 2007-11-15 07:36 Message: Logged In: YES user_id=1831942 Originator: YES 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. ---------------------------------------------------------------------- Comment By: Charles Groves (cgroves) Date: 2007-11-12 00:01 Message: Logged In: YES user_id=1174327 Originator: NO __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. ---------------------------------------------------------------------- Comment By: Mehendran (mehendran) Date: 2007-10-26 03:37 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 01:36 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-14 22:22 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 07:50 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 00:03 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 09:14 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 09:12 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 |
From: SourceForge.net <no...@so...> - 2007-12-20 09:04:27
|
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: 6 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-12-20 14:34 Message: Logged In: YES user_id=1831942 Originator: YES 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 ---------------------------------------------------------------------- Comment By: Charles Groves (cgroves) Date: 2007-12-20 14:25 Message: Logged In: YES user_id=1174327 Originator: NO 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. ---------------------------------------------------------------------- Comment By: Mehendran (mehendran) Date: 2007-12-20 12:10 Message: Logged In: YES user_id=1831942 Originator: YES I have done regrtest and the problem is solved. File Added: new_collections_v3.diff ---------------------------------------------------------------------- Comment By: Charles Groves (cgroves) Date: 2007-12-09 04:59 Message: Logged In: YES user_id=1174327 Originator: NO test_cfgparser still fails on its own and when I run regrtest with the new patch. Are you not seeing that failure? ---------------------------------------------------------------------- Comment By: Mehendran (mehendran) Date: 2007-12-07 19:44 Message: Logged In: YES user_id=1831942 Originator: YES 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 ---------------------------------------------------------------------- Comment By: Charles Groves (cgroves) Date: 2007-12-01 02:04 Message: Logged In: YES user_id=1174327 Originator: NO 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. ---------------------------------------------------------------------- Comment By: Mehendran (mehendran) Date: 2007-11-15 18:24 Message: Logged In: YES user_id=1831942 Originator: YES 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. ---------------------------------------------------------------------- Comment By: Mehendran (mehendran) Date: 2007-11-15 18:06 Message: Logged In: YES user_id=1831942 Originator: YES 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. ---------------------------------------------------------------------- Comment By: Charles Groves (cgroves) Date: 2007-11-12 10:31 Message: Logged In: YES user_id=1174327 Originator: NO __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. ---------------------------------------------------------------------- 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 |
From: SourceForge.net <no...@so...> - 2007-12-30 23:52:14
|
Patches item #1804242, was opened at 2007-09-28 09:01 Message generated for change (Comment added) made by cgroves 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: Closed >Resolution: Accepted >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: Charles Groves (cgroves) Date: 2007-12-30 18:52 Message: Logged In: YES user_id=1174327 Originator: NO Applied in r3916. Thanks! ---------------------------------------------------------------------- Comment By: Mehendran (mehendran) Date: 2007-12-20 04:04 Message: Logged In: YES user_id=1831942 Originator: YES 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 ---------------------------------------------------------------------- Comment By: Charles Groves (cgroves) Date: 2007-12-20 03:55 Message: Logged In: YES user_id=1174327 Originator: NO 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. ---------------------------------------------------------------------- Comment By: Mehendran (mehendran) Date: 2007-12-20 01:40 Message: Logged In: YES user_id=1831942 Originator: YES I have done regrtest and the problem is solved. File Added: new_collections_v3.diff ---------------------------------------------------------------------- Comment By: Charles Groves (cgroves) Date: 2007-12-08 18:29 Message: Logged In: YES user_id=1174327 Originator: NO test_cfgparser still fails on its own and when I run regrtest with the new patch. Are you not seeing that failure? ---------------------------------------------------------------------- Comment By: Mehendran (mehendran) Date: 2007-12-07 09:14 Message: Logged In: YES user_id=1831942 Originator: YES 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 ---------------------------------------------------------------------- Comment By: Charles Groves (cgroves) Date: 2007-11-30 15:34 Message: Logged In: YES user_id=1174327 Originator: NO 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. ---------------------------------------------------------------------- Comment By: Mehendran (mehendran) Date: 2007-11-15 07:54 Message: Logged In: YES user_id=1831942 Originator: YES 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. ---------------------------------------------------------------------- Comment By: Mehendran (mehendran) Date: 2007-11-15 07:36 Message: Logged In: YES user_id=1831942 Originator: YES 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. ---------------------------------------------------------------------- Comment By: Charles Groves (cgroves) Date: 2007-11-12 00:01 Message: Logged In: YES user_id=1174327 Originator: NO __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. ---------------------------------------------------------------------- Comment By: Mehendran (mehendran) Date: 2007-10-26 03:37 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 01:36 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-14 22:22 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 07:50 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 00:03 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 09:14 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 09:12 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 |