Skip to content

Refactor Filters #2178

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.

Sign up for GitHub

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

Jump to bottom
Closed
Bibo-Joshi opened this issue Nov 3, 2020 · 9 comments · Fixed by #2759
Closed

Refactor Filters #2178

Bibo-Joshi opened this issue Nov 3, 2020 · 9 comments · Fixed by #2759
Assignees
@harshil21
Labels
🛠 breaking change type: breaking 🛠 refactor change type: refactor
Milestone
v20.0a0

Comments

@Bibo-Joshi
Copy link
Member

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 Bibo-Joshi added the 🛠 refactor change type: refactor label Nov 3, 2020
@Bibo-Joshi Bibo-Joshi added this to the v14 milestone Nov 4, 2020
@Bibo-Joshi Bibo-Joshi mentioned this issue Dec 1, 2020
Merged
@Bibo-Joshi Bibo-Joshi added the 🛠 breaking change type: breaking label Dec 17, 2020
@Bibo-Joshi Bibo-Joshi changed the title Refactor Filters check mechanism Refactor Filters Jul 22, 2021
@Bibo-Joshi
Copy link
Member Author

Bibo-Joshi commented Jul 22, 2021 •
edited
Loading

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
Copy link
Member Author

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 :)

@Bibo-Joshi Bibo-Joshi mentioned this issue Oct 10, 2021
Closed
24 tasks
@jeffsieu
Copy link

Hi, can I have a go at this?

@Bibo-Joshi
Copy link
Member Author

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
Copy link
Member

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

@jeffsieu
Copy link

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
Copy link
Member

I like the KISS + shortcut solution

@Bibo-Joshi
Copy link
Member Author

@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 :)

@harshil21 harshil21 mentioned this issue Oct 25, 2021
Merged
@Bibo-Joshi
Copy link
Member Author

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.

@harshil21 harshil21 self-assigned this Oct 28, 2021
@harshil21 harshil21 mentioned this issue Oct 30, 2021
Merged
3 tasks
@harshil21 harshil21 linked a pull request Oct 30, 2021 that will close this issue
Overhaul of Filters #2759
Merged
3 tasks
@github-actions github-actions bot locked and limited conversation to collaborators Nov 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@harshil21 harshil21

Labels
🛠 breaking change type: breaking 🛠 refactor change type: refactor
Projects
None yet
Milestone
v20.0a0
Development

Successfully merging a pull request may close this issue.

Overhaul of Filters python-telegram-bot/python-telegram-bot
4 participants
@jeffsieu @Bibo-Joshi @Poolitzer @harshil21