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

Overhaul of Filters #2759

Merged
merged 69 commits into from
Nov 20, 2021
Merged

Overhaul of Filters #2759

merged 69 commits into from
Nov 20, 2021

Conversation

harshil21
Copy link
Member

@harshil21 harshil21 commented Oct 30, 2021

Breaking changes:

Also fixes #2281 (the problem was that we had duplicate docs in filters.)

Checklist for PRs

  • Added .. versionadded:: version, .. versionchanged:: version or .. deprecated:: version to the docstrings for user facing changes (for methods/class descriptions, arguments and attributes) How do we want to document this breaking change? Just a .. versionchanged:: on top of the filters module?
  • Created new or adapted existing unit tests

Notable changes:

  • Converted __new__ in BaseFilter to __init__ see: Overhaul of Filters #2759 (comment)

  • Made _UpdateType public, so users can now do: filters.UpdateType.MESSAGES.

  • deleted ChatType's filter() as discussed offline (reasoning was that Filters.chat_type was always True) .

  • All _ChatUserBaseFilter subclasses have a CAPS shortcut with allow_empty=True, we can discuss to remove this / keep it default (False). This allows a user to do something like filters.SENDER_CHAT work for both Channels and Supergroups.

    Edit: It was decided that those shortcuts won't inherit from _CUBF and instead be their own, see
    https://t.me/pythontelegrambotdev/261 for discussion.

  • Use a functools.partial() in DICE filter to simplify it for the user. See the examples section in Dice docs.
    Reverted. See: Overhaul of Filters #2759 (comment)

Minor changes:

  • remove the name argument in _Dice (to reduce repetition, it is now built from the emoji itself)
  • changed import of User, Chat in filters to TG{User, Chat} to avoid namespace conflicts.
  • The filters docs are now arranged according to how it appears in source.
  • Converted Optional[str] -> str in MimeType and Category (it was just plain wrong).
  • Rearrange filters in the source (right now they're all over the place). Should be done after a approving review since the diff is already messy.

Bibo-Joshi and others added 30 commits October 3, 2021 20:12
Co-authored-by: Bibo-Joshi <22366557+Bibo-Joshi@users.noreply.github.com>
Co-authored-by: Hinrich Mahler <22366557+Bibo-Joshi@users.noreply.github.com>
Co-authored-by: Bibo-Joshi <22366557+Bibo-Joshi@users.noreply.github.com>
Signed-off-by: starry69 <starry369126@outlook.com>
Co-authored-by: Hinrich Mahler <22366557+Bibo-Joshi@users.noreply.github.com>
Co-authored-by: Hinrich Mahler <22366557+Bibo-Joshi@users.noreply.github.com>
Co-authored-by: Harshil <37377066+harshil21@users.noreply.github.com>
Co-authored-by: Harshil <37377066+harshil21@users.noreply.github.com>
Co-authored-by: Harshil <37377066+harshil21@users.noreply.github.com>
Co-authored-by: poolitzer <25934244+Poolitzer@users.noreply.github.com>
Co-authored-by: Hinrich Mahler <22366557+Bibo-Joshi@users.noreply.github.com>
@harshil21
Copy link
Member Author

Something I recently noticed: A side effect of #2345 was that the filters docs stopped showing class signatures. See for e.g before -> after.
This was because we added a __new__ in BaseFilter, and sphinx seems to be picking the signature of that instead of the actual class.

So we can:

  1. Try to fix by changing something sphinx related
  2. Try to remove __new__ (which I was unsuccessful at since the same class variable / property / instance variable was being used causing lots of issues with slots)
  3. Don't fix since we document everything well enough anyway.

@Bibo-Joshi
Copy link
Member

Mh, this comes from the fact that Sphinx uses inspect.signature, which favor __new__ over __init__ (which makes sense …). We have the same for TelegraObject and BasePersistence.

I think for this PR we can leave it as is and rather convert this in another spin-off issue to think about __new__ in general. Some thoughts:

  1. For TelegramObject and BaseFilter we can drop the need for __new__ by just accepting that we need to call super().__init__. Sure, not having to do it nicer, but we'll survive. For TelegramObject we can even get away even easier with [DISCUSSION] Dataclasses #2698: if TelegramObject is a dataclass as well, the auto-generated init will automatically call super
  2. For BasePersistence we could move everything that currently happens in __new__ to __init__ that exists anyway. Alternatively, want to make the insert/replace_bot mechanism more explicit anyway in [FEATURE/DISCUSSION]: Allow fine tuning of persistence setup #2524 so we could switch to another approach for replacing/inserting - see e.g. [BUG] Persistence of Bots #1992

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.

nice updates! I left a number of more comments though I'd classify most of them as nitpicking by now 😃

telegram/ext/filters.py Outdated Show resolved Hide resolved
docs/source/telegram.ext.filters.rst Outdated Show resolved Hide resolved
telegram/ext/filters.py Show resolved Hide resolved
telegram/ext/_commandhandler.py Outdated Show resolved Hide resolved
telegram/ext/_messagehandler.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
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.

One detail, otherwise I'm happy :)
IIRC you'll sort the contents of filters.py now, right?

I'll force-push v14 to include the master changes later tonight.
If you want to, this could be a good chance to get to know rebasing :D basically, you (pull v14 and) run git rebase v14 and follow the comments in the command line. I guess there should be few to none merge conflicts :)
Alternatively, just merging v14 into this branch is fine as well ofc :)

PS: this article helped me quite well when working with rebase the first time

telegram/ext/filters.py Outdated Show resolved Hide resolved
@harshil21 harshil21 merged commit 4698bc4 into v14 Nov 20, 2021
@harshil21 harshil21 deleted the refactor-filters branch November 20, 2021 10:36
harshil21 added a commit that referenced this pull request Nov 20, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Nov 27, 2021
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.

[BUG] Sphinx warnings Refactor Filters