| msg309200 - (view) |
Author: Yahya Abou Imran (yahya-abou-imran) * |
Date: 2017-12-29 21:59 |
| Quoting my own post on python-ideas: After I generate an UML diagram from collections.abc, I found very strange that MappingView inherit from Sized instead of Collection (new in python 3.6). Yes, MappingView only define __len__ and not __iter__ and __contains__, but all of its subclasses define them (KeysView, ValuesView and ItemViews). I tried to run the tests in test/test_collections.py after making this change and on only one fail : Traceback (most recent call last): File "/usr/lib/python3.6/test/test_collections.py", line 789, in test_Collection self.assertNotIsInstance(x, Collection) AssertionError: dict_values([]) is an instance of <class 'collections.abc.Collection'> Wich is absolutely wrong, since in reality a dict_values instance has the behaviour of a Collection: >>> vals = {1:'a', 2: 'b'}.values() >>> 'a' in vals True >>> 'c' in vals False >>> len(vals) 2 >>> for val in vals: ... print(val) ... a b The only lack is that it doesn't define a __contains__ method: >>> '__contains__' in vals False It uses __iter__ to find the presence of the value. But, hey: we have register() for this cases! In fact, when MappingView inherit from Collection, dict_values is considered as a subclass of Collection since it's in the register of ValuesView, causing the above bug... So, the test have to be changed, and dict_values must be placed in the samples that pass the test, and not in the ones that fail it. |
|
|
| msg309203 - (view) |
Author: Yahya Abou Imran (yahya-abou-imran) * |
Date: 2017-12-29 22:19 |
| Quoting my own post on python-ideas: After I generate an UML diagram from collections.abc, I found very strange that MappingView inherit from Sized instead of Collection (new in python 3.6). Yes, MappingView only define __len__ and not __iter__ and __contains__, but all of its subclasses define them (KeysView, ValuesView and ItemViews). I tried to run the tests in test/test_collections.py after making this change and on only one fail : Traceback (most recent call last): File "/usr/lib/python3.6/test/test_collections.py", line 789, in test_Collection self.assertNotIsInstance(x, Collection) AssertionError: dict_values([]) is an instance of <class 'collections.abc.Collection'> Wich is absolutely wrong, since in reality a dict_values instance has the behaviour of a Collection: >>> vals = {1:'a', 2: 'b'}.values() >>> 'a' in vals True >>> 'c' in vals False >>> len(vals) 2 >>> for val in vals: ... print(val) ... a b The only lack is that it doesn't define a __contains__ method: >>> '__contains__' in vals False It uses __iter__ to find the presence of the value. But, hey: we have register() for this cases! In fact, when MappingView inherit from Collection, dict_values is considered as a subclass of Collection since it's in the register of ValuesView, causing the above bug... So, the test have to be changed, and dict_values must be placed in the samples that pass the test, and not in the ones that fail it. Here is a patch file for this. The only problem in this is that the MappingView won't be instatiable anymore because of the lack of __contains__ and __iter__. But since - if my understanding is correct - it's just an "utilty" super class for three other views (so is Collection for Set, Mapping and Sequence), it's not a big deal IMHO. |
|
|
| msg309204 - (view) |
Author: R. David Murray (r.david.murray) *  |
Date: 2017-12-29 22:39 |
| Well, it would be a backward compatibility problem at a minimum. Obviously things were designed this way. Have you found out why? Is there a consensus on python-ideas that this should be changed? If so, please link to the thread. |
|
|
| msg309205 - (view) |
Author: Yahya Abou Imran (yahya-abou-imran) * |
Date: 2017-12-29 23:08 |
| https://mail.python.org/pipermail/python-ideas/2017-December/048489.html Guido is waiting for objection on this... I have absolutly no idea why this decision was made. These tests (ttps://github.com/python/cpython/blame/master/Lib/test/test_collections.py#L794): non_col_iterables = [_test_gen(), iter(b''), iter(bytearray()), (x for x in []), dict().values()] for x in non_col_iterables: self.assertNotIsInstance(x, Collection) were written by Guido himself. |
|
|
| msg309206 - (view) |
Author: Yahya Abou Imran (yahya-abou-imran) * |
Date: 2017-12-29 23:24 |
| And I just saw that Raymon Hettinger made it inherting from Sized: https://github.com/python/cpython/blame/master/Lib/_collections_abc.py#L694 So I we were to ahve his opinion on this... |
|
|
| msg309271 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2017-12-31 09:39 |
| > After I generate an UML diagram from collections.abc, I found > very strange that MappingView inherit from Sized instead > of Collection (new in python 3.6). That isn't strange at all. MappingView predates Collection, so of course it didn't inherit from collection. > Yes, MappingView only define __len__ and not __iter__ > and __contains__, but all of its subclasses define them > (KeysView, ValuesView and ItemViews). It is irrelevant what extra behaviors subclasses may or may not add. The MappingView ABC is a public API and users are free to use it in other ways (as long as they stick with the publicly document API). For example, the following is allowed (even it seems odd to you): >>> from collections.abc import MappingView >>> mv = MappingView(['a', 'b']) >>> len(mv) # guaranteed 2 >>> repr(mv) # guaranteed "MappingView(['a', 'b'])" >>> 'a' in mv # not guaranteed Traceback (most recent call last): File "<pyshell#9>", line 1, in 'a' in mv TypeError: argument of type 'MappingView' is not iterable >>> list(mv) # not guaranteed Traceback (most recent call last): File "<pyshell#10>", line 1, in list(mv) TypeError: 'MappingView' object is not iterable |
|
|
| msg309272 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2017-12-31 09:57 |
| I concur with Raymond. MappingView is a public interface, not just a concrete implementation. Changing it will make third-party implementations of this interface not conforming it. |
|
|
| msg309280 - (view) |
Author: Yahya Abou Imran (yahya-abou-imran) * |
Date: 2017-12-31 12:50 |
| Hmm... Okay, I understand. So the only objection I have left, it's that ValuesView isn't passing the is instance of Collection test whereas it should, since he has the full behavior of one. It could be passed in its register to solve this. But it's not realy the same issue. Maybe I have to open a new one? |
|
|
| msg309435 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2018-01-03 21:14 |
| > Maybe I have to open a new one? No need. Just continue here. |
|
|
| msg309437 - (view) |
Author: Yahya Abou Imran (yahya-abou-imran) * |
Date: 2018-01-03 21:25 |
| I'm sorry, It's already done... https://bugs.python.org/issue32467 |
|
|