Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
Bibo-Joshi merged 18 commits intomasterfromfix-1562
May 10, 2020

Conversation

Bibo-Joshi
Copy link
Member

  1. Makesfilters.user.{user_ids, usernames} editable by:
    1. Makinguser_ids andusernames properties
    2. Adding locks for thread safety
  2. Adds tests for editing the attributes

If I got that right, thiscloses#1562

@Bibo-JoshiBibo-Joshi added this to the12.5 milestoneFeb 6, 2020
@Bibo-JoshiBibo-Joshi changed the titleFix 1562Make filters.user.{user_ids, usernames} mutableFeb 6, 2020
@Bibo-JoshiBibo-Joshi mentioned this pull requestFeb 21, 2020
8 tasks
@Bibo-Joshi
Copy link
MemberAuthor

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, asFilters.user was not really designed to be mutable.
Then again, I'm not very passionate about that change and can revert it.

@Bibo-JoshiBibo-Joshi changed the titleMake filters.user.{user_ids, usernames} mutableMake filters.user.{user_ids, usernames} editableApr 8, 2020
@tsnoamtsnoam changed the titleMake filters.user.{user_ids, usernames} editableAllow updating user_ids/usernames of a running filters.userApr 16, 2020
Copy link
Member

@tsnoamtsnoam 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

@Bibo-Joshi
Copy link
MemberAuthor

Two more things:

  1. I just noticed, thatFilters.chat currently does pretty much the same asFilters.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

  • I just noticed, thatFilters.chat currently does pretty much the same asFilters.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.

@tsnoam
Copy link
Member

  • I just noticed, thatFilters.chat currently does pretty much the same asFilters.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
MemberAuthor

  • I just noticed, thatFilters.chat currently does pretty much the same asFilters.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 withFilters.user, so it's a one time jobe :D

@tsnoam
Copy link
Member

LGTM

@Bibo-Joshi
Copy link
MemberAuthor

Fails unrelated. Merging.

@Bibo-JoshiBibo-Joshi merged commit613175b intomasterMay 10, 2020
@Bibo-JoshiBibo-Joshi deleted the fix-1562 branchMay 10, 2020 10:15
@tsnoamTelegram GithubBot Revised
Copy link
Member

@Bibo-Joshi 💥

@github-actionsgithub-actionsbot locked asresolvedand limited conversation to collaboratorsAug 17, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@tsnoamtsnoamtsnoam approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
12.6
Development

Successfully merging this pull request may close these issues.

[BUG] Change condition in Filters.user to avoid error with empty lists
2 participants
@Bibo-Joshi@tsnoam

[8]ページ先頭

©2009-2025 Movatter.jp