False positive __init__ method from base class is not called when the constructor raises NotImplementedError without using abc
#3975
Comments
|
@cpitclaudel thanks for your report. Why not using |
|
Are you thinking of this? from abc import ABCMeta, abstractmethod
class Base:
__metaclass__ = ABCMeta
@abstractmethod
def __init__(self, a: int, b: str):
pass
class Derived(Base):
def __init__(self, a: int, b: str):
super().__init__(a, b)
self.a = a + 1
self.b = b[::-1]That's what one of the answers on stackoverflow suggests, but it's a lot of cruft, and it doesn't prevent |
A different perspective: maybe there should be a pylint rule that suggests moving to an ABS instead of using NotImplementedError(); but this issue is about a different problem, namely pylint suggesting to call an |
|
@cpitclaudel indeed if you use metaclass this way: from abc import ABCMeta, abstractmethod
class Base(metaclass=ABCMeta):
@abstractmethod
def __init__(self, a: int, b: str):
pass
class Derived(Base):
def __init__(self, a: int, b: str):
super().__init__(a, b)
self.a = a + 1
self.b = b[::-1]
if __name__ == "__main__":
x = Base(3, "toto")and execute this program, you will get: Thus it is impossible to instantiate However i agree that |
👍 , though does the added ABCMeta stuff buy anything compared to the version that just throws an exception? Pylint currently recognizes methods that throw And having to call |
|
I agree with the need to ignore |
|
may someone add an high priority tag to this issue? This is nearly a bug. |
|
Hi @Py-GNU-Unix The high priority tag is for issue that have more than 10 thumbsup/heart reactions, but we're reviewing every contribution. If you want to contribute to this issue, it's going to get merged before high priority issues that do not get any attention. |
Pierre-Sassoulas
added
the
Needs PR
Pierre-Sassoulas
changed the title
__init__ method from base class is not called when the constructor raises NotImplementedError without using abc
Pierre-Sassoulas
added
False Positive 🦟
|
I think it's NOT an False Positive.
An good practice example from me: #!/usr/bin/env python3
# -*- coding: utf-8 -*-
"""Abstract class example."""
from abc import ABCMeta, abstractmethod
class Abstract(metaclass=ABCMeta):
"""Abstract class."""
@property
@abstractmethod
def message(self) -> str:
"""str: Return message."""
@abstractmethod
def __init__(self, message: str) -> None:
"""Abstract constructor.
Args:
message (str):
Message.
"""
class Concrete(Abstract):
"""Concrete class."""
@property
def message(self) -> str:
"""str: Return message."""
return self._message
def __init__(self, message: str) -> None:
"""Concrete constructor.
Args:
message (str):
Message.
"""
super().__init__(message)
self._message = message
print(Concrete("test").message)(using |
|
@vivi90 Your example is great, but this issue is about an |
Yes, thats WRONG behaviour but with the RIGHT goal. 😄 |
I think it makes sense to remove abstract methods from this check. Due to historic reasons That said, I think an argument can be made to create a code style check that suggests to inherit from |
I think that everything might be better than removing this check for abstract
Yes, i think the same.
Yes, exactly.
Discussing this in an own issue might be out of context. |
Whenever you implement anything in the abstract class but also raise UnimplementedError I don't think pylint should be able to recognise that. That's just an anti-pattern. Pylint's recognition of abstract methods and classes is actually quite old and robust so I don't think that should limit us. We tend to separate issues and messages from each other: users might not have all messages turned on so dependencies between messages are best avoided. Because of this we should take this message and ask ourselves: should If this then creates a false positive or negative we should fix our abstract recognition, but the premise of not raising is still valid imo. |
Oh no, that's not what i wanted to purpose. 😆
Yes, that's also not what i wanted to say.
hm okay so you mean if someone write something like this.. @abstractmethod
def __init__(self, message: str) -> None:
"""Abstract constructor.
Args:
message (str):
Message.
"""
this._message = messagethen this
Hmm, for |
No, as long as something "meaningful" happens in the method it is not considered abstract.
Yeah. So in such cases this shouldn't warn. |
That's exactly the point i am worried about.
So then i prefer the current behaviour of |
|
I have added a test to #7227 to make the new behaviour more explicit. It should be: class Base:
def __init__(self, param: int, param_two: str) -> None:
raise NotImplementedError()
class Derived(Base):
def __init__(self, param: int, param_two: str) -> None:
self.param = param + 1
self.param_two = param_two[::-1]
class AbstractBase(abc.ABC):
def __init__(self, param: int) -> None:
self.param = param + 1
def abstract_method(self) -> str:
"""This needs to be implemented."""
raise NotImplementedError()
class DerivedFromAbstract(AbstractBase):
def __init__(self, param: int) -> None: # [super-init-not-called]
print("Called")
def abstract_method(self) -> str:
return "Implemented"Whenever |
|
Thanks for your work @DanielNoord |
|
#7227 was merged so this should be available in |
Is your feature request related to a problem? Please describe
In the following example, Pylint flags the
Derived's__init__method because it doesn't callBases's__init__.This is discussed here: https://stackoverflow.com/questions/25072363/error-init-method-from-base-class-is-not-called-for-an-abstract-class
Describe the solution you'd like
Pylint could (should?) detect this case and not report an error.
Additional context
An easy fix is to remove the init in the base class, but it can be useful to have it there to define what signature
__init__should have.The text was updated successfully, but these errors were encountered: