-
-
Notifications
You must be signed in to change notification settings - Fork 29.2k
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
Comments
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? |
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. |
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. |
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 |
+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.) |
Good idea! I see Raymond's point about the name. How about .method_check? 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. |
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. |
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. |
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. |
I have updated the previous patch, by documenting the new class method. |
Can we have a formal patch review with a view to committing as this issue is referenced from bpo-9859. |
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__? |
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. |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: