classification
Title: change tracemalloc.BaseFilter to an abstract class
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.11
process
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: Nosy List: anton.gruebel, lukasz.langa, vstinner
Priority: normal Keywords: patch

Created on 2021-08-01 14:31 by anton.gruebel, last changed 2021-08-05 21:36 by lukasz.langa. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 27527 closed anton.gruebel, 2021-08-01 14:37
Messages (5)
msg398699 - (view) Author: Anton Grübel (anton.gruebel) * Date: 2021-08-01 14:31
during some work on typeshed I found the BaseFilter class in tracemalloc and it totally looks like and is used as a typical abstract class.

I will also directly create the PR :) if you think I'm missing something, I'm happy to hear some other thoughts.
msg398921 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2021-08-04 17:55
Many standard libraries, including new ones like asyncio, use `raise NotImplementedError` to denote methods that need re-implementation in subclasses. Some of those base classes even literally *called* Abstract and yet don't inherit from ABC (nor use ABCMeta). Do you intend to change all those as well?

While I agree it would be cleaner to have them defined as abstract, at the moment I'm not sure it's worth the churn.
msg398978 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-08-05 09:09
I don't see the benefit of the PR 27527. "BaseFilter" contains "Base" in its name and its only *private* method raises NotImplementedError. Maybe my design mistake (I wrote the tracemalloc module) was to expose it in the first place.

I suggest to leave the code as it is. As Łukasz wrote, there are other stdlib modules using "base" or "abstract" classes without using the abc module.
msg398982 - (view) Author: Anton Grübel (anton.gruebel) * Date: 2021-08-05 10:05
Even it is a private method, it is essential, when you use Snapshot.filter_traces(). Creating a Filter without implementing this method will just result in a runtime error.

https://github.com/python/cpython/blob/029cb4a6adacb650dbfc8ea71d2875ab771fe92e/Lib/tracemalloc.py#L442-L451

For me it feels like a bad practice to just recommend the developer to implement this method instead of enforcing it. Why is there a possibility to state clearly that a class is an abstract class and not use it? Even PEP20 states, "Explicit is better than implicit."

So why is there a way to create an abstract class in Python, just for fun? I don't think so, it is to make the intent very clear. As said, there is probably a lot of old code laying around in Python, which could be modernized by using functionality of newer Python versions, so why not change it? Is there actually any drawback in defining this class as an abstract class or do you actually gain more, because everyone directly understands that an abstract method has to be implemented in the subclass.

@Lukasz: if you ask me, I would change all of them, but I'm definitely lacking the skill and experience to find all those flaws and fix them. I'm happy to help and for me making code better even it is small is always appreciated, if you always think it is not worth the trouble or nothing really to gain, then it will stay that way and more and more of those constructs will arise. In Germany we have a saying, small cattle also make muck :D

But thanks for taking a look, even it was not worth the trouble. I had fun  creating the PR, but you can just do what you like with it.
msg399042 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2021-08-05 21:36
> if you ask me, I would change all of them

The problem with adopting an ABC base class for each such use case is two-fold:
- it changes the MRO putting the ABCMeta metaclass in which might make existing subclasses suddenly incompatible; and
- is costly in terms of performance even if subclasses are still compatible.

In other words, putting ABC in isn't a transparent operation. This is one reason why it wasn't included in asyncio code.

Since we're not reacting to any breakage here, I'd feel best leaving this alone. Thanks for your care Anton!
History
Date User Action Args
2021-08-05 21:36:22lukasz.langasetstatus: open -> closed
resolution: rejected
messages: + msg399042

stage: patch review -> resolved
2021-08-05 10:05:56anton.gruebelsetmessages: + msg398982
2021-08-05 09:09:38vstinnersetnosy: + vstinner
messages: + msg398978
2021-08-04 17:55:21lukasz.langasetnosy: + lukasz.langa
messages: + msg398921
2021-08-01 14:37:36anton.gruebelsetkeywords: + patch
stage: patch review
pull_requests: + pull_request26042
2021-08-01 14:31:36anton.gruebelcreate