Repository navigation
gh-158911 Docs: Correct and clarify NotImplementedError usage guidelines - #158912
clauspruefer wants to merge 1 commit into
Conversation
Update the documentation for `NotImplementedError` to specify that it should be raised in non-abstract methods of user-defined base classes rather than abstract ones. Additionally, add a caution note explaining that adding `NotImplementedError` inside methods decorated with `abc.abstractmethod` is redundant, as the ABC metaclass enforcement prevents instantiation without concrete child implementations, rendering the base method body uncalled.
|
All commit authors signed the Contributor License Agreement. |
bedevere-app
Bot
added
awaiting review
docs
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The caution incorrectly claims abstract method bodies cannot be called through overriding methods.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Clarifies guidance for using NotImplementedError in base classes.
Changes:
- Recommends use in non-abstract methods requiring overrides.
- Adds cautionary guidance about
abc.abstractmethod.
| File | Description |
|---|---|
Doc/builtins/exceptions.rst |
Updates NotImplementedError usage documentation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| Methods decorated with :func:`abc.abstractmethod` designate a member function | ||
| as abstract, which prompts the ABC metaclass enforcement mechanism to verify | ||
| that a concrete implementation resides within the instantiated subclass. | ||
| Consequently, the Python interpreter invokes the overridden child class implementation | ||
| directly; the original base class method body remains uncalled during regular | ||
| polymorphic execution, rendering the inclusion of a :exc:`NotImplementedError` | ||
| entirely superfluous and redundant. |
There was a problem hiding this comment.
An abstract method is a method that is declared without an implementation (it has no code body). It defines a method's signature—> such as its name, parameters, and return type—but leaves the actual logic to be defined by its subclasses.
Calling a base class method via super() from within its overridden implementation is controversial when discussing what constitutes a strictly 'abstract' method.
Documentation build overview
|
picnixz
left a comment
picnixz
left a comment
There was a problem hiding this comment.
I don't think it's wrong though. A class can be considered abstract either from a semantical PoV or from a runtime PoV (inheriting abc.ABC). In the former case, raising NotImplementedError is relevant. The PoCs in the issues are also inaccurate:
Calling i.meow() raises: TypeError: Can't instantiate abstract class Tiger without an implementation for abstract method 'meow', the NotImplementedError exception code never will be executed.
That's not true. It's the instantiation of i that already raises the exception, not calling the method.
FTR, I fail to see the relation with class methods. And I don't think the docs are semantically wrong.
| meant to be supported at all -- in that case either leave the operator / | ||
| method undefined or, if a subclass, set it to :data:`None`. | ||
|
|
||
| .. caution:: |
There was a problem hiding this comment.
We don't want this paragraph. This leaks implementation details and relates to a concept that is not about exceptions specifically.
| derived classes to override the method, or while the class is being | ||
| developed to indicate that the real implementation still needs to be added. | ||
| This exception is derived from :exc:`RuntimeError`. In user-defined base | ||
| classes, any **non**-abstract method should raise this exception when derived |
There was a problem hiding this comment.
That's not necessarily true, see
cpython/Lib/_collections_abc.py
Lines 450 to 452 in fb313a3
This makes the intent clearer instead of having a pass statement or a ... statement and allows one to remove the decorator if necessary.
There was a problem hiding this comment.
That's not necessarily true, see
cpython/Lib/_collections_abc.py
Lines 450 to 452 in fb313a3
for instance.This makes the intent clearer instead of having a
passstatement or a...statement and allows one to remove the decorator if necessary.
No, this is wrong. An abstract method never should contain code.
There was a problem hiding this comment.
In Python you must have one code. A pass statement remains something. So sorry but I'm not accepting this change. Clarity is better than purity in the language and NotImplementedError predates ABCs. ABCs also add overhead at runtime while raising NotImplementedError directly (without any abc.abstractmethod decorator) is the only way to convene the intent of an abstract method.
The page about exceptions is not about ABC only. It's for anyone wanting to define an abstract method.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
Marking as draft till CLA is signed. |
|
Closing because the docs for the exception specifically are correct. If you use ABCs, that's different. |
Reference (Details)
Fixes #158911
Implementation
Update the
/Doc/builtins/exceptions.rstdocumentation forNotImplementedErrorto specify that it should be raised in non-abstract methods of user-defined base classes rather than abstract ones.Additionally, add a caution note explaining that adding
NotImplementedErrorinside methods decorated withabc.abstractmethodis redundant, as the ABC metaclass enforcement prevents instantiation without concrete child implementations, rendering the base method body uncalled.