Skip to content

Conversation

@GauthamramRavichandran
Copy link
Contributor

@GauthamramRavichandran GauthamramRavichandran commented Oct 10, 2020

  • Created a new supergroup filter
  • Specified that group filter will accept both normal groups and supergroups

closes #2126

@GauthamramRavichandran
Copy link
Contributor Author

Fixes one of #2126

@Poolitzer
Copy link
Member

Hi.

While it is correct that you implemented proposal 1, we agreed on doing 3 in the linked issue. Can you overhaul your PR to reflect this?

Also, you need to write unitests which test your changes, dont forget that. If you install pre-commit hooks as explained in our contribution.md guide, that will be done locally before you commit \o/

@GauthamramRavichandran
Copy link
Contributor Author

  • Created ChatTypes with channel, group, supergroup and private. Added unit tests for the same

Copy link
Member

@Bibo-Joshi Bibo-Joshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates. Unfortunately, most of the changes discussed in #2126 §3 are still missing. Sorry, if my explanations weren't detailed enough … I tried to clarify what's missing in some comments. The filter that comes closest to what is needed here is the Filters.dice, maybe have a look at that :) If you have questions, feel free to reach out on TG.

Finally, recalling @Poolitzer s idea, I think it would be good to add an attribute Filters.groups (plural) for both super- and normal groups to chat_type

PS: Just writing down a thought as "fyi": I had the idea that one could also make the chat-type filters attriubtes of Filters.chat, which filters for specific chat ids. That would have the benefit that we don't have a Filters.chat_type which admittetly has a small use case. However, Filters.chat is already quite complex and I fear that users might get confused about what it acutally filters for. Keeping filtering for ids and types separate is probably better.

@Poolitzer
Copy link
Member

Poolitzer commented Oct 11, 2020

+1 for filtering types and ids seperately in two filters please thank you

@GauthamramRavichandran
Copy link
Contributor Author

could someone implement the __call__ method for _ChatType, thank you

@Bibo-Joshi
Copy link
Member

Thinking about it more, passing args to Filters.chat_type might be a bit over the top. For now I just added the deprecation warnings.

@Poolitzer
Copy link
Member

what is stopping us from merging?

Copy link
Member

@Poolitzer Poolitzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two doc strings are wrong, otherwise great change!

@Bibo-Joshi Bibo-Joshi merged commit 237e73b into python-telegram-bot:master Oct 29, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Oct 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEATURE] Filter for chat types

3 participants