Skip to content
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

You don't need to raise NotImplementedError for abstract methods #100400

Open
Xophmeister opened this issue Dec 21, 2022 · 2 comments
Open

You don't need to raise NotImplementedError for abstract methods #100400

Xophmeister opened this issue Dec 21, 2022 · 2 comments

Comments

@Xophmeister
Copy link

Xophmeister commented Dec 21, 2022

Documentation

In the documentation for NotImplementedError, we see the following:

.. exception:: NotImplementedError
This exception is derived from :exc:`RuntimeError`. In user defined base
classes, abstract methods should raise this exception when they require
derived classes to override the method, or while the class is being
developed to indicate that the real implementation still needs to be added.

If "abstract base classes" are meant to mean ABCs from the standard library, this is unnecessary as the ABC metaclass will prohibit instantiation of any derived class that doesn't implement an abstract method. Indeed, adding a raise would be dead code that would flaunt, say, coverage metrics.

@stevendaprano
Copy link
Member

stevendaprano commented Dec 21, 2022

I think the purpose of raising NotImplementedError is to protect against calling the abstract method from the class like this:

>>> from abc import ABC, abstractmethod
>>> 
>>> class Animal(ABC):
...     @abstractmethod
...     def move(self):
...         raise NotImplementedError
... 
>>> class Human(Animal):
...     def move(self):
...         print("human move")
... 
>>> Animal.move(Human())
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 4, in move
NotImplementedError

If we removed the NotImplementedError, calling the abstract move method would silently do nothing.

I think the statement should stay, or even be made more prominent in the docs. I see lots of abstract methods which wrongly (in my opinion) just have pass as the body.

Does anyone know whether linters check for this?

@sobolevn
Copy link
Member

sobolevn commented Dec 22, 2022

I cannoy say for all linters, but mypy does not catch it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants