Repository navigation
Add ABCMeta.has_methods and tests that use it #53940
Description
| BPO | 9731 |
|---|---|
| Nosy | @rhettinger, @terryjreedy, @benjaminp, @merwok, @durban, @PCManticore, @serhiy-storchaka |
| Files |
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']
Activity
stutzbach commentedon Sep 1, 2010
In bpo-9212, I mused:
É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?
gvanrossum commentedon Sep 1, 2010
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 commentedon 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 commentedon Sep 1, 2010
Yeah, that makes sense. You could put those checks in the unittests.
stutzbach commentedon Sep 1, 2010
Re-opening and re-titling the issue to that effect.
Proposed syntax and usage:
Usage within the unit tests:
[-]ABCMeta.register should verify that methods are present[/-][+]Add ABCMeta.has_methods and tests that use it[/+]rhettinger commentedon Sep 1, 2010
+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 commentedon Sep 3, 2010
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 commentedon Sep 3, 2010
I agree that my original name proposal is terrible. :-) method_check and verify_full_api both look fine to me.
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 commentedon 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 commentedon 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 commentedon Apr 24, 2014
I have updated the previous patch, by documenting the new class method.
BreamoreBoy commentedon 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 commentedon Mar 10, 2015
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 commentedon 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.