Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Collection to collections.abc and typing #71785

Closed
brettcannon opened this issue Jul 23, 2016 · 26 comments
Closed

Add Collection to collections.abc and typing #71785

brettcannon opened this issue Jul 23, 2016 · 26 comments
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@brettcannon
Copy link
Member

brettcannon commented Jul 23, 2016

BPO 27598
Nosy @gvanrossum, @brettcannon, @rhettinger, @NeilGirdhar, @ilevkivskyi, @bharel
Files
  • collection.diff
  • doc_changes.diff
  • collection_neil_combined.diff
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2016-08-23.18:07:48.791>
    created_at = <Date 2016-07-23.15:46:49.458>
    labels = ['type-feature', 'library']
    title = 'Add Collection to collections.abc and typing'
    updated_at = <Date 2016-08-23.19:30:53.470>
    user = 'https://github.com/brettcannon'

    bugs.python.org fields:

    activity = <Date 2016-08-23.19:30:53.470>
    actor = 'levkivskyi'
    assignee = 'none'
    closed = True
    closed_date = <Date 2016-08-23.18:07:48.791>
    closer = 'gvanrossum'
    components = ['Library (Lib)']
    creation = <Date 2016-07-23.15:46:49.458>
    creator = 'brett.cannon'
    dependencies = []
    files = ['44144', '44146', '44148']
    hgrepos = []
    issue_num = 27598
    keywords = ['patch']
    message_count = 26.0
    messages = ['271088', '271094', '271110', '271112', '271170', '271191', '271963', '273009', '273012', '273037', '273038', '273069', '273075', '273079', '273082', '273102', '273116', '273164', '273228', '273467', '273478', '273494', '273495', '273499', '273500', '273512']
    nosy_count = 8.0
    nosy_names = ['gvanrossum', 'brett.cannon', 'rhettinger', 'python-dev', 'NeilGirdhar', 'levkivskyi', 'srkunze', 'bar.harel']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue27598'
    versions = ['Python 3.6']

    @brettcannon
    Copy link
    Member Author

    brettcannon commented Jul 23, 2016

    See the discussion on python-ideas entitled "An ABC representing "Iterable, Sized, Container"".

    @brettcannon brettcannon added the stdlib Python modules in the Lib dir label Jul 23, 2016
    @gvanrossum
    Copy link
    Member

    gvanrossum commented Jul 23, 2016

    I'm thinking that it should be called Collection after all.

    --Guido (mobile)

    @rhettinger
    Copy link
    Contributor

    rhettinger commented Jul 23, 2016

    Does all of the recognition patterns in typing.py also need to be in collections.abc?

    AFAICT, the current collections.abc classes for Sized, Iterable, and Container almost never get used (possibly because they don't provide any useful mixin methods). In the interest of maximizing the ratio of useful ABCs (like MutableMapping), could we defer expanding collections.abc until we've seen some real use cases?

    @gvanrossum
    Copy link
    Member

    gvanrossum commented Jul 23, 2016

    It would be a deviation from existing practice for a collection ABC to
    exist in typing but not in collections.abc, and it would mean the
    inheritance tree for the types in typing.py would be different from
    that in collections.abc.

    These ABCs, while not so useful by themselves, are building blocks for
    the more useful ABCs. They also give people the right terminology.
    That arguably is the biggest problem here -- it's clear that people
    desperately want to be able to talk about the common API for sets and
    sequences, which happens to be __len__, __iter__ and __contains__. But
    it's also clear that people often believe that that common API is
    called Iterable, which it isn't.

    @rhettinger
    Copy link
    Contributor

    rhettinger commented Jul 24, 2016

    In putting together a patch for this, I noticed that the __subclasshook__ classmethods don't compose well. I'm unclear about the semantics about what these hooks are supposed to mean for subclasses and whether there is a bug in the existing code for Set, Sequence, and Mapping. Each of those inherits from Sized, Iterable, and Container, but only the Sized.__subclasshook__ is visible to the subclass. Also, the code for Sized.__subclasshook__ has an identity check for Sized which doesn't seem correct from the point-of-view of the subclasses.

    @gvanrossum
    Copy link
    Member

    gvanrossum commented Jul 25, 2016

    Regarding the design of __subclasshook__, these two flaws kind of cancel each other out. The idea is that if you have a "one-trick pony" class whose issubclass() check must verify that the purported sublass implements a specific method (e.g. __hash__), you don't want that subclass check to be inherited by a subclass. I suppose perhaps this ought to have been done by making ABCMeta.__subclasscheck__ check for the __subclasshook__ in the class dict, but (for reasons I've forgotten) it's not doing it that way, instead just calling cls.__subclasscheck__. All known __subclasshook__ implementations (those in collections.abc anyway :-) compensate for this by returning NotImplemented if the class for which they are being called isn't exactly the class in which they are defined. The final piece of the puzzle is object.__subclasshook__(), which always returns NotImplemented.

    (NOTE: When reading the above paragraph, be careful to distinguish between __subclasscheck__, which is invoked by issubclass() and isinstance(), and __subclasshook__, which is only invoked by ABCMeta.__subclasscheck__().)

    Here's an example showing why we don't want __subclasshook__ to be inherited. Suppose we have a class SupportsInt, like this:

    class SupportsInt(ABC):
        @classmethod
        def __subclasshook__(cls, C):
            return hasattr(C, '__int__')

    Now suppose we had a concrete class inheriting from this:

    class MyInteger(SupportsInt):
        def __int__(self):
            return 0

    Now, alas, everything with an __int__ method is considered to be a MyInteger, for example isinstance(0, MyInteger) returns True.

    So what should Collection.__subclasshook__ do? It could do something like this:

    class Collection(Set, Iterable, Container):
        @classmethod
        def __subclasshook__(cls, C):
            if cls is not Collection:
                return NotImplemented
            for base in Set, Iterable, Container:
                ok = base.__subclasshook__(C)
                if ok != True: return False
            return True

    (Untested, hopefully you get the idea. Hope this helps.)

    @srkunze
    Copy link
    Mannequin

    srkunze mannequin commented Aug 4, 2016

    I am way too late to the discussion but I also support the term "collections". I think it also helps developers coming from a C# background:

    https://msdn.microsoft.com/library/92t2ye13(v=vs.110).aspx

    @gvanrossum
    Copy link
    Member

    gvanrossum commented Aug 18, 2016

    Ping -- I'd really like to see this happening, with "Collection" as the name.

    @NeilGirdhar
    Copy link
    Mannequin

    NeilGirdhar mannequin commented Aug 18, 2016

    @gvanrossum is there any reason that subclasshook is implemented by overriding instead of cooperation? E.g.,:

    class Sized(metaclass=ABCMeta):
    
        @classmethod
        def __subclasshook__(cls, C):
            return (super().__subclasshook__(C) and 
                    any("__len__" in B.__dict__ for B in C.__mro__))

    etc. And then Collection does not need to implement subclasshook since its base classes cooperate?

    @gvanrossum
    Copy link
    Member

    gvanrossum commented Aug 18, 2016

    Why don't you give it a try? I'd be happy to review a patch from you.

    @gvanrossum
    Copy link
    Member

    gvanrossum commented Aug 18, 2016

    I thought about this some more. It won't work because when a class method
    is called via super(), 'cls' is still set to the derived class (and in
    general for class methods that's the right thing to do, just as it is for
    regular methods that self is still the actual object). If you read my
    previous long comment you'll understand that all the __subclasshook__
    methods carefully return NotImplemented immediately if their 'cls' argument
    is not the class where they are defined. So it won't work.

    I also don't see the use case (you're not supposed to do this at home,
    basically).

    @ilevkivskyi
    Copy link
    Member

    ilevkivskyi commented Aug 19, 2016

    I submit a simple solution in line with __subclasshook__ of other ABCs.
    I added several tests, it looks like it is doing everything right.

    Please review.

    @gvanrossum
    Copy link
    Member

    gvanrossum commented Aug 19, 2016

    LGTM, but I'm on vacation through Monday.

    @NeilGirdhar
    Copy link
    Mannequin

    NeilGirdhar mannequin commented Aug 19, 2016

    Great patch. Shouldn't Sequence be a "Reversible, Collection"?

    @NeilGirdhar
    Copy link
    Mannequin

    NeilGirdhar mannequin commented Aug 19, 2016

    (added the documentation changes)

    @ilevkivskyi
    Copy link
    Member

    ilevkivskyi commented Aug 19, 2016

    Thank you Neil, I agree that Sequence is a reversible collection.

    I combined you documentation patch with updated _collections_abc and updated test_functools (it tests MRO) into a single patch.

    @NeilGirdhar
    Copy link
    Mannequin

    NeilGirdhar mannequin commented Aug 19, 2016

    Given issue http://bugs.python.org/issue27802, it might be worth considering that all Collections implement __eq__ and __ne__, so maybe these should be abstract methods on Collection?

    @NeilGirdhar NeilGirdhar mannequin changed the title Add SizedIterable to collections.abc and typing Add Collection to collections.abc and typing Aug 19, 2016
    @NeilGirdhar
    Copy link
    Mannequin

    NeilGirdhar mannequin commented Aug 19, 2016

    (never mind about the comparison operators :) Turns out that would break backwards compatibility.)

    @gvanrossum
    Copy link
    Member

    gvanrossum commented Aug 20, 2016

    No on adding __eq__.

    If another core dev is available please go ahead and commit.

    @gvanrossum gvanrossum changed the title Add Collection to collections.abc and typing Add SizedIterable to collections.abc and typing Aug 20, 2016
    @bharel
    Copy link
    Mannequin

    bharel mannequin commented Aug 23, 2016

    I still believe "Reiterable" better demonstrates the concept.

    When you request a Reiterable as a function parameter or assert if something is a Reiterable the other side knows exactly what you mean.

    A "Collection" is way more ambiguous - if you create an object that acts like range() but you can cycle over it more than once I wouldn't exactly call it a collection. I would though call it a Reiterable and it would be clear for any Python programmer familiar with the concept of iterators.

    I believe this is a funny case in which the naming is more important than the implementation as it will turn into a term or concept that will be further used in many places to come.

    @gvanrossum
    Copy link
    Member

    gvanrossum commented Aug 23, 2016

    ReIterable and Collection are different concepts.

    • ReIterable just implements __iter__ but promises certain semantics.
    • Collection implements __iter__, __contains__ and __len__.

    If we had ReIterable I would probably decree that Collection inherits from it (practicality beats purity).

    But I don't think ReIterable by itself would do much good; apart from a few "show-off" hacks, any reasonable ReIterable would also be able to implement __contains__ and __len__ easily.

    And I would certainly call range() a Collection.

    @gvanrossum gvanrossum changed the title Add SizedIterable to collections.abc and typing Add Collection to collections.abc and typing Aug 23, 2016
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 23, 2016

    New changeset acd9a465b373 by Guido van Rossum in branch 'default':
    bpo-27598: Add Collections to collections.abc.
    https://hg.python.org/cpython/rev/acd9a465b373

    @gvanrossum
    Copy link
    Member

    gvanrossum commented Aug 23, 2016

    I'll keep this open until I've also merged typing.py.

    @gvanrossum
    Copy link
    Member

    gvanrossum commented Aug 23, 2016

    changeset: 102867:355fa8bd8d9d
    branch: 3.5
    parent: 102860:e3466a556d81
    user: Guido van Rossum <guido@python.org>
    date: Tue Aug 23 11:01:50 2016 -0700
    summary: A new version of typing.py from https://github.com/python/typing.

    changeset: 102868:013bb690c24a
    tag: tip
    parent: 102866:acd9a465b373
    parent: 102867:355fa8bd8d9d
    user: Guido van Rossum <guido@dropbox.com>
    date: Tue Aug 23 11:06:30 2016 -0700
    summary: A new version of typing.py from https://github.com/python/typing. (Merge 3.5->3.6)

    @gvanrossum gvanrossum added the type-feature A feature request or enhancement label Aug 23, 2016
    @gvanrossum
    Copy link
    Member

    gvanrossum commented Aug 23, 2016

    Actually there's one TODO left: the docs for typing (at least in 3.6)
    should mention Collection.

    @ilevkivskyi
    Copy link
    Member

    ilevkivskyi commented Aug 23, 2016

    Guido, thank you for committing! There are also some other omissions in docs, Type[C], @overload, there is bpo-26141 to track this. I will submit a patch later. Until 3.6beta1 I would like to focus on PEP-526.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants