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

Allow updating user_ids/usernames of a running filters.user #1757

Merged
merged 18 commits into from
May 10, 2020
Merged

Conversation

Bibo-Joshi
Copy link
Member

  1. Makes filters.user.{user_ids, usernames} editable by:
    1. Making user_ids and usernames properties
    2. Adding locks for thread safety
  2. Adds tests for editing the attributes

If I got that right, this closes #1562

@Bibo-Joshi Bibo-Joshi added this to the 12.5 milestone Feb 6, 2020
@Bibo-Joshi Bibo-Joshi changed the title Fix 1562 Make filters.user.{user_ids, usernames} mutable Feb 6, 2020
@Bibo-Joshi Bibo-Joshi mentioned this pull request Feb 21, 2020
8 tasks
@Bibo-Joshi
Copy link
Member Author

Maybe a comment on the change to sets is in order: I did this because we need only user id/name only once and that way removing a user from the filter is easier.
This kind of breaks backwards compitibility, but only kind of, as Filters.user was not really designed to be mutable.
Then again, I'm not very passionate about that change and can revert it.

@Bibo-Joshi Bibo-Joshi changed the title Make filters.user.{user_ids, usernames} mutable Make filters.user.{user_ids, usernames} editable Apr 8, 2020
@tsnoam tsnoam changed the title Make filters.user.{user_ids, usernames} editable Allow updating user_ids/usernames of a running filters.user Apr 16, 2020
Copy link
Member

@tsnoam tsnoam left a comment

Choose a reason for hiding this comment

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

see comments on the code

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

Two more things:

  1. I just noticed, that Filters.chat currently does pretty much the same as Filters.user. I think it would be good to update it as well. I'll try and set up a base class for the two of them in order to reduce code duplication
  2. As it wasn't really discussed yet: We're okay with switching from lists to sets for usernames/ids? Users shouldn't have fiddled with them anyway, so if they if that's their problem? :D

@tsnoam
Copy link
Member

tsnoam commented May 1, 2020

  • I just noticed, that Filters.chat currently does pretty much the same as Filters.user. I think it would be good to update it as well. I'll try and set up a base class for the two of them in order to reduce code duplication

good idea. go for it.

  • As it wasn't really discussed yet: We're okay with switching from lists to sets for usernames/ids? Users shouldn't have fiddled with them anyway, so if they if that's their problem? :D
    as i see it, whether it's a list or set is an implementation detail. not exposed to the user. (the user may pass anything which we can iterate. we'll convert it into a set)

so what i'm saying, sets are good and for large numbers will provide better performance. in small numbers it wouldn't matter if it's a list or set.

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
telegram/ext/filters.py Outdated Show resolved Hide resolved
telegram/ext/filters.py Outdated Show resolved Hide resolved
telegram/ext/filters.py Outdated Show resolved Hide resolved
telegram/ext/filters.py Show resolved Hide resolved
telegram/ext/filters.py Show resolved Hide resolved
telegram/ext/filters.py Outdated Show resolved Hide resolved
telegram/ext/filters.py Show resolved Hide resolved
@tsnoam
Copy link
Member

tsnoam commented May 1, 2020

  • I just noticed, that Filters.chat currently does pretty much the same as Filters.user. I think it would be good to update it as well. I'll try and set up a base class for the two of them in order to reduce code duplication

good idea. go for it.

on a second thought, having a general solution might be over engineering.
IMHO, you can just copy/paste the code.

Co-authored-by: Noam Meltzer <tsnoam@gmail.com>
@Bibo-Joshi
Copy link
Member Author

  • I just noticed, that Filters.chat currently does pretty much the same as Filters.user. I think it would be good to update it as well. I'll try and set up a base class for the two of them in order to reduce code duplication

good idea. go for it.

on a second thought, having a general solution might be over engineering.
IMHO, you can just copy/paste the code.

Okay. But then I'll wait until we're happy with Filters.user, so it's a one time jobe :D

telegram/ext/filters.py Outdated Show resolved Hide resolved
telegram/ext/filters.py Outdated Show resolved Hide resolved
telegram/ext/filters.py Outdated Show resolved Hide resolved
telegram/ext/filters.py Outdated Show resolved Hide resolved
telegram/ext/filters.py Show resolved Hide resolved
telegram/ext/filters.py Show resolved Hide resolved
telegram/ext/filters.py Outdated Show resolved Hide resolved
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
telegram/ext/filters.py Outdated Show resolved Hide resolved
@tsnoam
Copy link
Member

tsnoam commented May 9, 2020

LGTM

@Bibo-Joshi
Copy link
Member Author

Fails unrelated. Merging.

@Bibo-Joshi Bibo-Joshi merged commit 613175b into master May 10, 2020
@Bibo-Joshi Bibo-Joshi deleted the fix-1562 branch May 10, 2020 10:15
Copy link
Member

tsnoam commented May 10, 2020

@Bibo-Joshi 💥

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 17, 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.

[BUG] Change condition in Filters.user to avoid error with empty lists
2 participants