Refactor Filters

[Bibo-Joshi]

Currently filters check updates via the __call__ method. When switching from function-based filters to class-based filters, this was mainly done to maintain backwards compatibility with filter(update). However with some of the new filters functionalities, this leads to ugly workarounds like

python-telegram-bot/telegram/ext/filters.py

Lines 329 to 350 in 8e7c0d6

	class _Text(MessageFilter):
	name = 'Filters.text'
	class _TextStrings(MessageFilter):
	def __init__(self, strings: List[str]):
	self.strings = strings
	self.name = 'Filters.text({})'.format(strings)
	def filter(self, message: Message) -> bool:
	if message.text:
	return message.text in self.strings
	return False
	def __call__( # type: ignore[override]
	self, update: Union[Update, List[str]]
	) -> Union[bool, '_TextStrings']:
	if isinstance(update, Update):
	return self.filter(update.effective_message)
	return self._TextStrings(update)
	def filter(self, message: Message) -> bool:
	return bool(message.text)

I therefore propose to refactor to something along the lines of

class BaseFilter:
    def __call__(self, *args, **kwargs):
        return self.__class__(*args, **kwargs)

    @abstractmethod
    def check_update(update/method):
        ...

This matches the Handler.check_update interface and allows for filters having a custom __init__, wich can still be called through instances of those filters.

However, this is somewhat breaking: While BaseFilter.__call__ is undocumented, people might still have Filters.existing_filter(update) somewhere within their custom filters. So maybe this is better suited for a major release.

[Bibo-Joshi]

Following up on this, I was thinking about why we have the Filters class at all. IISC it's mostly a history reason: First filters where just enums. After that they were functions and those were grouped as static functions in the Filters class, which was originally part of the messagehandler.py module. The docstring even said "convenience namespace":

python-telegram-bot/telegram/ext/messagehandler.py

Lines 26 to 30 in 2c2aaea

	class Filters(object):
	"""
	Convenient namespace (class) & methods for the filter funcs of the
	MessageHandler class.
	"""

---

Regarding the namespace, I see no notable added benefit of grouping the filters in a Filters class instead of just grouping them in a filters module - all one would have to change would be from

from telegram.ext import Filters
Filters.text

to

from telegram.ext import filters
filters.text

(A nice side effect of this would be probably behave better)

---

More importantly, we have by now made a full transition to class-based filters, where we have three different cases:

1. filters that we provide is instances, i.e. ready for use (e.g. Filters.location)
2. filters that need to be initialized (e.g. Filters.user(user_id))
3. filters that can do both (e.g. Filters.text or Filters.text(some_list))

Because we use lower_snail_case namings for all of these, there is sometimes some confusion about how to use the filters in the cases 2 and 3. Also it obviously breaks naming conventions. I therefore propose to change the namings as follows:

1. For case 1, use CAPS_SNAIL_CASE as usual for constants
2. for case 2, use CamelCase as usual for classes

Case 3 is a bit more tricky and I see different options:

1. As close to the current situation as possible

Sticking to the Filters.text example, we could add filters.TEXT and make it callable, where filters.TEXT(*args) would return a new instance of a filters._PrivateTextFilter class - basically as described above.

2. The hacky solution

We can add a filters.Text class, where filters.Text.filter can be used both as classmethod and as instance method. This can be achieved with descriptiors, as explained on this SO question. That way, one could use both MessageHandler(filters.Text, callback) and MessageHandler(filters.Text(some_list), callback). Note that this behavior would only work for filters where we apply the descriptor and e.g. it wouldn't work for custom filters out of the box.

3. KISS

Add filters.Text as class. We can then either tell users to use filters.Text(), or add filters.TEXT as shortcut for filters.Text()

---

Personally, I'm somewhat in favor of the KISS-solution, as IMO it offers the cleanest API to the user.

[Bibo-Joshi]

Another comment: currently in setup.cfg we make mypy shut up for a lot of errors in filters.py. A PR for this should remove that setting and try to use as few type-ignores as possible :)

[jeffsieu]

Hi, can I have a go at this?

[Bibo-Joshi]

Hi. IIRC we haven't made a final decision on this, which is why I didn't tag it for hacktoberfest. If @Poolitzer @starry69 @harshil21 are on board with the KISS variant, I'd be happy to assign you to this issue :)

[harshil21]

KISS FTW. If applied consistently throughout filters, it shouldn't cause much confusion than what is already there.

[jeffsieu]

Hi. IIRC we haven't made a final decision on this, which is why I didn't tag it for hacktoberfest. If @Poolitzer @starry69 @harshil21 are on board with the KISS variant, I'd be happy to assign you to this issue :)

Oh I see... Thanks for the info!

[Poolitzer]

I like the KISS + shortcut solution

[Bibo-Joshi]

@jeffsieu with Pool and harshil agreeing to the KISS solution, we can go ahead with this. If you would like to PR for this, please leave a short comment here so that I can assign you :)

[Bibo-Joshi]

Another refactoring idea: I was thinking about the nested filters, e.g. Filters.document and Filters.chat, which both have multiple attributes that again are filters. However those two cases are notably different, because Filters.document itself only allows messages with a document while Filters.chat essentially allows all messages since every message has a chat type.

For Filters.document the KISS approach described above should still make sense: Convert it to a class filters.Document, which has attributes that are either classes again (filters.Document.FileExtension) or attributes of type MessageFilter (filters.Document.APPLICATION).

However, Filters.ChatType would have no real inherit value - it's mostly there to structure the name space. Therefore I propose to make it an Enum. This gives us a handy way of structuring the name spaces while keeping out any trivial code.