This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: MappingView must inherit from Collection instead of Sized
Type: enhancement Stage: resolved
Components: Versions: Python 3.7, Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: rhettinger Nosy List: r.david.murray, rhettinger, serhiy.storchaka, yahya-abou-imran
Priority: normal Keywords: patch

Created on 2017-12-29 21:59 by yahya-abou-imran, last changed 2022-04-11 14:58 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
mypatch.patch yahya-abou-imran, 2017-12-29 22:19
Pull Requests
URL Status Linked Edit
PR 5047 closed python-dev, 2017-12-29 23:46
Messages (10)
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) * (Python committer) 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) * (Python committer) 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 <module>
        '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 <module>
        list(mv)
    TypeError: 'MappingView' object is not iterable
msg309272 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) 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) * (Python committer) 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
History
Date User Action Args
2022-04-11 14:58:56adminsetgithub: 76630
2018-01-12 05:54:57rhettingersetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2018-01-03 21:25:54yahya-abou-imransetmessages: + msg309437
2018-01-03 21:14:21rhettingersetmessages: + msg309435
2017-12-31 12:50:25yahya-abou-imransetmessages: + msg309280
2017-12-31 09:57:17serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg309272
2017-12-31 09:39:38rhettingersetassignee: rhettinger

messages: + msg309271
nosy: + rhettinger
2017-12-29 23:46:40python-devsetstage: patch review
pull_requests: + pull_request4928
2017-12-29 23:24:31yahya-abou-imransetmessages: + msg309206
2017-12-29 23:08:41yahya-abou-imransetmessages: + msg309205
2017-12-29 22:39:59r.david.murraysetnosy: + r.david.murray
messages: + msg309204
2017-12-29 22:19:30yahya-abou-imransetfiles: + mypatch.patch
keywords: + patch
messages: + msg309203
2017-12-29 21:59:35yahya-abou-imrancreate