classification
Title: Add ABCMeta.has_methods and tests that use it
Type: enhancement Stage: patch review
Components: Library (Lib), Tests Versions: Python 3.5
process
Status: open Resolution: accepted
Dependencies: Superseder:
Assigned To: stutzbach Nosy List: Claudiu.Popa, benjamin.peterson, daniel.urban, eric.araujo, laura, rhettinger, serhiy.storchaka, stutzbach, terry.reedy
Priority: normal Keywords: patch

Created on 2010-09-01 13:02 by stutzbach, last changed 2018-11-06 18:26 by gvanrossum.

Files
File name Uploaded Description Edit
abc_9731.patch Claudiu.Popa, 2013-11-17 12:19 review
issue9731.patch Claudiu.Popa, 2014-04-24 21:37 review
Messages (14)
msg115293 - (view) Author: Daniel Stutzbach (stutzbach) (Python committer) Date: 2010-09-01 13:02
In Issue 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 (Issue 9212)

range objects claims to support the Sequence ABC but don't provide the count and index methods (Issue 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?
msg115305 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2010-09-01 14:49
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.
msg115309 - (view) Author: Daniel Stutzbach (stutzbach) (Python committer) Date: 2010-09-01 15:01
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.
msg115312 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2010-09-01 15:16
> 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.
msg115316 - (view) Author: Daniel Stutzbach (stutzbach) (Python committer) Date: 2010-09-01 15:31
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
msg115321 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2010-09-01 18:09
+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.)
msg115487 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2010-09-03 19:32
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.
msg115492 - (view) Author: Daniel Stutzbach (stutzbach) (Python committer) Date: 2010-09-03 19:48
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.
msg115497 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2010-09-03 20:14
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.
msg203142 - (view) Author: Claudiu Popa (Claudiu.Popa) * (Python triager) Date: 2013-11-17 12:19
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.
msg217138 - (view) Author: Claudiu Popa (Claudiu.Popa) * (Python triager) Date: 2014-04-24 21:37
I have updated the previous patch, by documenting the new class method.
msg221822 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2014-06-28 23:53
Can we have a formal patch review with a view to committing as this issue is referenced from #9859.
msg237744 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-10 10:52
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__?
msg241021 - (view) Author: Laura Rupprecht (laura) * Date: 2015-04-14 20:47
Related item: Added a helper function to verify API match of two modules, addressing issue9859.

It checks for a closer match but mentioned this ticket from the thread.
History
Date User Action Args
2018-11-06 18:26:50gvanrossumsetnosy: - gvanrossum
2018-11-06 02:57:38xtreaksetpull_requests: - pull_request9633
2018-11-05 15:14:46BreamoreBoysetnosy: - BreamoreBoy
2018-11-05 02:36:07Jonathan.Gossagesetpull_requests: + pull_request9633
2015-04-14 20:47:41laurasetnosy: + laura
messages: + msg241021
2015-03-10 10:52:45serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg237744
2014-08-25 08:58:40Claudiu.Popasetstage: needs patch -> patch review
2014-06-28 23:53:34BreamoreBoysetnosy: + BreamoreBoy
messages: + msg221822
2014-04-24 21:37:55Claudiu.Popasetfiles: + issue9731.patch

messages: + msg217138
versions: + Python 3.5, - Python 3.4
2013-11-17 12:19:14Claudiu.Popasetfiles: + abc_9731.patch
versions: + Python 3.4, - Python 3.2
nosy: + Claudiu.Popa

messages: + msg203142

keywords: + patch
2010-09-04 11:45:18daniel.urbansetnosy: + daniel.urban
2010-09-03 20:14:55eric.araujosetmessages: + msg115497
2010-09-03 19:48:28stutzbachsetmessages: + msg115492
2010-09-03 19:32:23terry.reedysetnosy: + terry.reedy
messages: + msg115487
2010-09-01 18:09:31rhettingersetnosy: + rhettinger
messages: + msg115321
2010-09-01 15:31:15stutzbachsetstatus: closed -> open

assignee: stutzbach
components: + Tests
title: ABCMeta.register should verify that methods are present -> Add ABCMeta.has_methods and tests that use it
nosy: gvanrossum, benjamin.peterson, stutzbach, eric.araujo
messages: + msg115316
resolution: rejected -> accepted
stage: needs patch
2010-09-01 15:16:22gvanrossumsetmessages: + msg115312
2010-09-01 15:01:58stutzbachsetmessages: + msg115309
2010-09-01 14:49:15gvanrossumsetstatus: open -> closed
resolution: rejected
messages: + msg115305
2010-09-01 13:02:32stutzbachcreate