False Positives for @abstractmethod

Help Wanted
1

Hi. I reported some false positives a while ago, which said that abstract methods should raise NotImplementedError, even though the class can’t even be instantiated when it inherits from abc.ABC.
I was contacted by mail about this:

Hey Hinrich,

I noticed you reported a false positive for PTC-W0053 - Abstract method does not raise NotImplementedError.

Thanks for providing a very detailed comment.

The issue was raised for the following snippet in the file telegram/ext/filters.py:


@abstractmethod
def __call__(self, update: Update) -> Optional[Union[bool, Dict]]:
    ...

This issue is raised when an abstract method is left not implemented using ... or pass.

I agree with your comment that a class with an abstract method cannot be initialized.

Raising NotImplementedError improves the code readability. It gives a clear indication and

makes it explicit that the method needs to be implemented in the subclass. It is a sort of

TODO marker.

Let me know what are your thoughts on this use case.

Thanks,

Faisal Riyaz
Language Engineering, DeepSource

My reply:

Hi Faisal,

thanks for reaching out.
However, I tend to disagree. IMHO the @abstractmethod (and even the ..., if you will), are clear indications that the method has to be implemented. Adding raise NotImplementedError can on the contrary give the impression that the method actually does something.

Moreover, for abstract classes that are part of a public API, the user usually won’t see the code anyway, while any decent IDE will complain (before run-time) if you try to subclass without implementing all the abtsract methods.

I’d agree that an error should not be raised, if the body of an abstract method contains only raise NotImplementedError, but it shouldn’t me mandatory.

Unfortunately I never heard back on this. Is there any update on the topic? The issue still persists and is one of only two things left before I can actually integrate DS into my project …

2

Hey @Bibo-Joshi!

We are yet to take this up. I am expecting this to be resolved within the next few days.
I’ll drop an update here as soon as we get this and the docstring issue fixed.

3

Update – this has been fixed!
Please let me know here if you face any more issues related to this particular check.

1 Like