-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
bpo-41905: added abc.update_abstractmethods #22485
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
Changes from all commits
f65b0f8
c04ba74
ea69ae6
8b76715
61c4f40
a5a9fd7
5e1fffc
2f95b84
3986c98
4f3d846
74473c8
92326c9
1e47ee4
740183f
114028e
ba8df08
29dba37
779e6cf
eea97f4
8eec6c2
5698153
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -122,6 +122,44 @@ def _abc_caches_clear(cls): | |
_reset_caches(cls) | ||
|
||
|
||
def update_abstractmethods(cls): | ||
"""Recalculate the set of abstract methods of an abstract class. | ||
|
||
If a class has had one of its abstract methods implemented after the | ||
class was created, the method will not be considered implemented until | ||
this function is called. Alternatively, if a new abstract method has been | ||
added to the class, it will only be considered an abstract method of the | ||
class after this function is called. | ||
|
||
This function should be called before any use is made of the class, | ||
usually in class decorators that add methods to the subject class. | ||
|
||
Returns cls, to allow usage as a class decorator. | ||
|
||
If cls is not an instance of ABCMeta, does nothing. | ||
""" | ||
if not hasattr(cls, '__abstractmethods__'): | ||
# We check for __abstractmethods__ here because cls might by a C | ||
# implementation or a python implementation (especially during | ||
# testing), and we want to handle both cases. | ||
return cls | ||
|
||
abstracts = set() | ||
# Check the existing abstract methods of the parents, keep only the ones | ||
# that are not implemented. | ||
for scls in cls.__bases__: | ||
for name in getattr(scls, '__abstractmethods__', ()): | ||
value = getattr(cls, name, None) | ||
if getattr(value, "__isabstractmethod__", False): | ||
abstracts.add(name) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add tests for deeper inheritance - I think iterating over
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It actually is- we are assured that the parent's
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added a test case regardless There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, interesting. How about this case? I think you might need to break after the first hit for the method:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is expected (albeit confusing) behavior. Since abstraction status cannot change after subclassing, A is still considered abstract, even though its method is implemented. However, since namespace resolution occurs through the In essence, abstract methods shouldn't be changed after the class is used, so this use case is not handled by this implementation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, I disagree with the check for empty Note that it's not technically true that the abstractness cannot change once subclassed -- it may be undocumented but one could just write FWIW I do agree that the code should assume that the abstractness of the bases is correct. If some base lies about it, well, Garbage In, Garbage Out. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think abc.update_abstractmethods(A) would have any impact here, A never changed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I was commenting on your earlier example which has My disagreement was with this quote from @bentheiii:
I agree with @iritkatriel that adding that test would be useful (I didn't verify -- maybe it's already added?). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ah, in that case it raises an exception that A has been subclasses so you can't update it anymore. I think there's a test for that already but if not then there should be. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see now - you're suggesting to remove that exception. In that case this test is probably needed. |
||
# Also add any other newly added abstract methods. | ||
for name, value in cls.__dict__.items(): | ||
if getattr(value, "__isabstractmethod__", False): | ||
abstracts.add(name) | ||
cls.__abstractmethods__ = frozenset(abstracts) | ||
return cls | ||
bentheiii marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
|
||
class ABC(metaclass=ABCMeta): | ||
"""Helper class that provides a standard way to create an ABC using | ||
inheritance. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -488,6 +488,155 @@ class C(with_metaclass(abc_ABCMeta, A, B)): | |
pass | ||
self.assertEqual(C.__class__, abc_ABCMeta) | ||
|
||
def test_update_del(self): | ||
class A(metaclass=abc_ABCMeta): | ||
@abc.abstractmethod | ||
def foo(self): | ||
pass | ||
|
||
del A.foo | ||
self.assertEqual(A.__abstractmethods__, {'foo'}) | ||
self.assertFalse(hasattr(A, 'foo')) | ||
|
||
abc.update_abstractmethods(A) | ||
|
||
self.assertEqual(A.__abstractmethods__, set()) | ||
A() | ||
|
||
|
||
def test_update_new_abstractmethods(self): | ||
class A(metaclass=abc_ABCMeta): | ||
@abc.abstractmethod | ||
def bar(self): | ||
pass | ||
|
||
@abc.abstractmethod | ||
def updated_foo(self): | ||
pass | ||
|
||
A.foo = updated_foo | ||
abc.update_abstractmethods(A) | ||
self.assertEqual(A.__abstractmethods__, {'foo', 'bar'}) | ||
msg = "class A with abstract methods bar, foo" | ||
self.assertRaisesRegex(TypeError, msg, A) | ||
|
||
def test_update_implementation(self): | ||
class A(metaclass=abc_ABCMeta): | ||
@abc.abstractmethod | ||
def foo(self): | ||
pass | ||
|
||
class B(A): | ||
pass | ||
|
||
msg = "class B with abstract method foo" | ||
self.assertRaisesRegex(TypeError, msg, B) | ||
self.assertEqual(B.__abstractmethods__, {'foo'}) | ||
|
||
B.foo = lambda self: None | ||
|
||
abc.update_abstractmethods(B) | ||
|
||
B() | ||
self.assertEqual(B.__abstractmethods__, set()) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you need this test case to cover the first loop:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @iritkatriel What does that test case accomplish other than showing that FooABC is now in an inconsistent state? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gvanrossum In this comment I left out the update_abstractmethods, see Ben's version which he committed as test_update_del. Without this test the first loop in update_abstractmethods was not exercised. |
||
def test_update_as_decorator(self): | ||
class A(metaclass=abc_ABCMeta): | ||
@abc.abstractmethod | ||
def foo(self): | ||
pass | ||
|
||
def class_decorator(cls): | ||
cls.foo = lambda self: None | ||
return cls | ||
|
||
@abc.update_abstractmethods | ||
@class_decorator | ||
class B(A): | ||
pass | ||
|
||
B() | ||
self.assertEqual(B.__abstractmethods__, set()) | ||
|
||
def test_update_non_abc(self): | ||
class A: | ||
pass | ||
|
||
@abc.abstractmethod | ||
def updated_foo(self): | ||
pass | ||
|
||
A.foo = updated_foo | ||
abc.update_abstractmethods(A) | ||
A() | ||
self.assertFalse(hasattr(A, '__abstractmethods__')) | ||
|
||
def test_update_del_implementation(self): | ||
class A(metaclass=abc_ABCMeta): | ||
@abc.abstractmethod | ||
def foo(self): | ||
pass | ||
|
||
class B(A): | ||
def foo(self): | ||
pass | ||
|
||
B() | ||
|
||
del B.foo | ||
|
||
abc.update_abstractmethods(B) | ||
|
||
msg = "class B with abstract method foo" | ||
self.assertRaisesRegex(TypeError, msg, B) | ||
|
||
def test_update_layered_implementation(self): | ||
class A(metaclass=abc_ABCMeta): | ||
@abc.abstractmethod | ||
def foo(self): | ||
pass | ||
|
||
class B(A): | ||
pass | ||
|
||
class C(B): | ||
def foo(self): | ||
pass | ||
|
||
C() | ||
|
||
del C.foo | ||
|
||
abc.update_abstractmethods(C) | ||
|
||
msg = "class C with abstract method foo" | ||
self.assertRaisesRegex(TypeError, msg, C) | ||
|
||
def test_update_multi_inheritance(self): | ||
class A(metaclass=abc_ABCMeta): | ||
@abc.abstractmethod | ||
def foo(self): | ||
pass | ||
|
||
class B(metaclass=abc_ABCMeta): | ||
def foo(self): | ||
pass | ||
|
||
class C(B, A): | ||
@abc.abstractmethod | ||
def foo(self): | ||
pass | ||
|
||
self.assertEqual(C.__abstractmethods__, {'foo'}) | ||
|
||
del C.foo | ||
|
||
abc.update_abstractmethods(C) | ||
|
||
self.assertEqual(C.__abstractmethods__, set()) | ||
|
||
C() | ||
|
||
|
||
class TestABCWithInitSubclass(unittest.TestCase): | ||
def test_works_with_init_subclass(self): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
A new function in abc: *update_abstractmethods* to re-calculate an abstract class's abstract status. In addition, *dataclass* has been changed to call this function. |
Uh oh!
There was an error while loading. Please reload this page.