-
Notifications
You must be signed in to change notification settings - Fork 6k
add supergroup filter #2128
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
add supergroup filter #2128
Conversation
|
Fixes one of #2126 |
|
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 |
|
There was a problem hiding this 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.
|
+1 for filtering types and ids seperately in two filters please thank you |
|
could someone implement the |
|
Thinking about it more, passing args to |
|
what is stopping us from merging? |
Poolitzer
left a comment
There was a problem hiding this 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!
closes #2126