Skip to content
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.

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

Refactor msg_in #1631

Merged
merged 4 commits into from
Nov 29, 2019
Merged

Refactor msg_in #1631

merged 4 commits into from
Nov 29, 2019

Conversation

Bibo-Joshi
Copy link
Member

As discussed offline, this refactors the Filters.msg_in filter to work as __call__ of Filters.text, thus closing #1625
I encountered two (and a half) obstacles:

  • BaseFilters __call__ is used to perform the update checks, i.e. it executes filter(). To allow for Filters.text({'foo', 'bar'}) anyway, Filters.texts __call__ now checks, if the argument is of type Update. If it's not, it returns an instance of a subclass that works like the former msg_in. An alternative way would be to do something like

    def __call__(self, update=None, iterable=None):
        if update:
            self.filter(update)
        else:
            return _TextIterable(iterable)
    

    However calling Filters.text(iterable={'foo', 'bar'}) seemed neither convenient nor intuitive for me. Also, usually you won't have to call Filter.text(update) manually anyway …

  • To recreate the caption filter functionality, I needed to add the base class Filters.caption that allows only messages that have a caption. Including caption filtering into Filters.text like

    Filters.text({'foo', 'bar'}, caption=True)
    

    didn't seem very natural to me …
    If you feel like Filters.caption is unneeded, we can just remove it. The main usecase for this PR is Filters.text(buttons) anyway …

  • I wasn't very original in naming the subclasses (_TextIterable and _CaptionIterable). If you have a better naming idea, please tell :)

@Bibo-Joshi Bibo-Joshi added this to the 12.3 milestone Nov 20, 2019
@Bibo-Joshi
Copy link
Member Author

Bibo-Joshi commented Nov 20, 2019

About failing tests:

  • Py2: Something about pinning messages. Seems unrelated. will restart.
  • Codecov: 🤷‍♂️

telegram/ext/filters.py Show resolved Hide resolved
telegram/ext/filters.py Outdated Show resolved Hide resolved
telegram/ext/filters.py Outdated Show resolved Hide resolved
@Bibo-Joshi
Copy link
Member Author

@Eldinnie Are you okay with the changes? And do you have any clue, what codecov wants?

@Eldinnie
Copy link
Member

Codecov is telling us that we did not test all branches:
image
It's telling you that the test if it's an update_filter is obsolete, because it never is (at least never tested). That's why the diff_target is not 100%

Anyway I'll allow it for now :D

@Eldinnie Eldinnie merged commit 5e8a961 into master Nov 29, 2019
@Eldinnie Eldinnie deleted the refactor_msg_in branch November 29, 2019 12:09
Bibo-Joshi added a commit that referenced this pull request Feb 6, 2020
tsnoam pushed a commit that referenced this pull request Feb 8, 2020
* Make Filters.command only accept MessageEntitie commands

* Add option to filters.command to allow cmds anywhere in the message

* Make codecov happy, also retroactive for #1631
@Bibo-Joshi Bibo-Joshi mentioned this pull request Apr 2, 2020
18 tasks
@github-actions github-actions bot locked and limited conversation to collaborators Aug 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants