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 ABCMeta.has_methods and tests that use it #53940

Open
stutzbach mannequin opened this issue Sep 1, 2010 · 14 comments
Open

Add ABCMeta.has_methods and tests that use it #53940

stutzbach mannequin opened this issue Sep 1, 2010 · 14 comments
Labels
stdlib Python modules in the Lib dir tests Tests in the Lib/test dir type-feature A feature request or enhancement

Comments

@stutzbach
Copy link
Mannequin

stutzbach mannequin commented Sep 1, 2010

BPO 9731
Nosy @rhettinger, @terryjreedy, @benjaminp, @merwok, @durban, @PCManticore, @serhiy-storchaka
Files
  • abc_9731.patch
  • issue9731.patch
  • 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 = None
    created_at = <Date 2010-09-01.13:02:32.027>
    labels = ['tests', 'type-feature', 'library']
    title = 'Add ABCMeta.has_methods and tests that use it'
    updated_at = <Date 2018-11-06.18:26:50.717>
    user = 'https://bugs.python.org/stutzbach'

    bugs.python.org fields:

    activity = <Date 2018-11-06.18:26:50.717>
    actor = 'gvanrossum'
    assignee = 'stutzbach'
    closed = False
    closed_date = None
    closer = None
    components = ['Library (Lib)', 'Tests']
    creation = <Date 2010-09-01.13:02:32.027>
    creator = 'stutzbach'
    dependencies = []
    files = ['32666', '35027']
    hgrepos = []
    issue_num = 9731
    keywords = ['patch']
    message_count = 14.0
    messages = ['115293', '115305', '115309', '115312', '115316', '115321', '115487', '115492', '115497', '203142', '217138', '221822', '237744', '241021']
    nosy_count = 9.0
    nosy_names = ['rhettinger', 'terry.reedy', 'benjamin.peterson', 'stutzbach', 'eric.araujo', 'daniel.urban', 'Claudiu.Popa', 'serhiy.storchaka', 'laura']
    pr_nums = []
    priority = 'normal'
    resolution = 'accepted'
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue9731'
    versions = ['Python 3.5']

    @stutzbach
    Copy link
    Mannequin Author

    stutzbach mannequin commented Sep 1, 2010

    In bpo-9212, I mused:

    I sort of wonder if .register() should verify that the concrete class
    provides all of the methods of the ABC.

    Éric Araujo suggested I open that as an issue for consideration.

    I have found a few bugs in the standard library where a concrete class did not actually implement all of the methods of the ABC that it purported to implement. Specifically:

    dict's keys view claims to implement the Set ABC but doesn't provide the is_disjoint method (bpo-9212)

    range objects claims to support the Sequence ABC but don't provide the count and index methods (bpo-9213)

    Should ABCMeta.register verify that the type implements all of the appropriate methods?

    On the one hand, that would prevent similar bugs from sneaking by. On the other hand, it's extra code that would get executed every time Python started (site.py imports os.py which imports _abcoll.py).

    Thoughts?

    @stutzbach stutzbach mannequin added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Sep 1, 2010
    @gvanrossum
    Copy link
    Member

    That would be too hard-headed. A complaint I often hear about Interfaces in Java is that classes have a hard time if they choose not to implement a certain method. It also would not be enough -- there are tons of ways you would be able to satisfy such a check to the letter while still breaking any call.

    It may be a good idea for a lint-style tool to warn about these though.

    @stutzbach
    Copy link
    Mannequin Author

    stutzbach mannequin commented Sep 1, 2010

    Would it be useful to provide a separate function to perform the check, which could be used by lint-style tools as well as automated tests?

    It would be great if every call to .register in the standard library had a corresponding test that verified that the appropriate methods were defined. That would prevent future oversights where methods are missing entirely.

    @gvanrossum
    Copy link
    Member

    Would it be useful to provide a separate function to perform the check, which could be used by lint-style tools as well as automated tests?

    It would be great if every call to .register in the standard library had a corresponding test that verified that the appropriate methods were defined.  That would prevent future oversights where methods are missing entirely.

    Yeah, that makes sense. You could put those checks in the unittests.

    @stutzbach
    Copy link
    Mannequin Author

    stutzbach mannequin commented Sep 1, 2010

    Re-opening and re-titling the issue to that effect.

    Proposed syntax and usage:

    # in Lib/abc.py
    class ABCMeta(type):
        # ...
    
        def has_methods(cls, subclass):
            "Returns True iff subclass implements the appropriate methods"
            # ...

    Usage within the unit tests:

    # In Lib/test/test_collections.py
        def test_methods(self):
            self.assertTrue(Sequence.has_methods(range))
            self.assertTrue(MutableSet.has_methods(set))
            # ... and many more

    @stutzbach stutzbach mannequin added the tests Tests in the Lib/test dir label Sep 1, 2010
    @stutzbach stutzbach mannequin reopened this Sep 1, 2010
    @stutzbach stutzbach mannequin changed the title ABCMeta.register should verify that methods are present Add ABCMeta.has_methods and tests that use it Sep 1, 2010
    @stutzbach stutzbach mannequin self-assigned this Sep 1, 2010
    @rhettinger
    Copy link
    Contributor

    +1 for the basic idea.

    It could use a better name, Sequence.has_methods(range) reads backwards to me. Perhaps something like: Sequence.implemented_by(range) or Sequence.verify_full_api(range) or some such.

    Also, when the tests get added, they should go in the test file for the implementing class (like test_range for example.)

    @terryjreedy
    Copy link
    Member

    Good idea!

    I see Raymond's point about the name. How about .method_check?
    To me Sequence.method_check(range) means "Abstract Seqeunce class, please method-check the concrete range class."

    If Sequence.register(range) is in the range source file, I would expect to find the test of the range class in test_range.

    If all collection registrations were bundled together in collections.py, I think I would expect the tests to be in test_collections. But I could still be presuaded that the range method check should be in test_range where all the individual methods are (should be!) tested.

    @stutzbach
    Copy link
    Mannequin Author

    stutzbach mannequin commented Sep 3, 2010

    I agree that my original name proposal is terrible. :-) method_check and verify_full_api both look fine to me.

    If all collection registrations were bundled together in
    collections.py, I think I would expect the tests to be in
    test_collections. But I could still be presuaded that the range method
    check should be in test_range where all the individual methods are
    (should be!) tested.

    All of the registrations are bundled together in _abcoll.py, which collections.py imports into collections.py's namespace. (It's in a separate file to get around some bootstrapping issues.)

    I favor putting them in test_collections to make it slightly easier to check for a 1:1 correlation between registrations and tests, but it's not something I feel strongly about.

    @merwok
    Copy link
    Member

    merwok commented Sep 3, 2010

    I wanted to propose the name check_methods, but then I thought that some people could read too much in that name, for example that arguments and return value are checked for correct type and value (see how Web people misunderstand “HTML validation”). That said, “check” promises less than “validate”, and ABCs being a middle-to-advanced topic, we could trust people using this method to read the docstring and not expect too much.

    @PCManticore
    Copy link
    Mannequin

    PCManticore mannequin commented Nov 17, 2013

    Hello! Here's a preliminary implementation, with tests for collections ABC. It doesn't have any docs yet, though. I chose the name verify_full_api, but I'm ok with check_methods as well.

    @PCManticore
    Copy link
    Mannequin

    PCManticore mannequin commented Apr 24, 2014

    I have updated the previous patch, by documenting the new class method.

    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented Jun 28, 2014

    Can we have a formal patch review with a view to committing as this issue is referenced from bpo-9859.

    @serhiy-storchaka
    Copy link
    Member

    What if abstract class contains not only abstract methods, but default implementation of some methods? Should the predicate test if tested class contain these methods?

    For example, the Sequence ABC contains default implementation of __iter__. Should the predicate test that concrete sequence class implements __iter__?

    @Laura
    Copy link
    Mannequin

    Laura mannequin commented Apr 14, 2015

    Related item: Added a helper function to verify API match of two modules, addressing bpo-9859.

    It checks for a closer match but mentioned this ticket from the thread.

    @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 tests Tests in the Lib/test dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants