Skip to content

Conversation

@marc0777
Copy link
Contributor

Added regex matching for message caption in Filters.regex.

I noticed that the regex filter only checks inside the message text, so if it's an image or a document it will be ignored, even if inside the caption the content could match.

Since we can already differentiate text messages and other contents using filters, I think the regex filter should work regardless of the type of message.

Added regex matching for message caption in Filters.regex.
@Bibo-Joshi
Copy link
Member

Hi. Thanks for the PR! I do see the use case for regexing a caption, but the change you propose would be breaking. I.e. MessageHandler(Filters.regex(…), …) currently only accepts text messages and with your PR it would suddently also accept media.

My first idea would be to move it to a standalone Filters.caption_regex like for Filters.caption_entitiy. Then you could still do Filters.regex(…) | Filters.caption_regex(…) to allow both.
The only downside I see is that people might get confused that Filters.photo & Filters.regex will not do the expected thing, while Filters.photo & Filters.caption_regex does. But I guess I can live with that.

Maybe @Poolitzer has an opinion on that?

In any case, we'd need unittests (see Codecov failure) - maybe have a look at tests/test_filters.py for inspiration :)

@Poolitzer
Copy link
Member

The only downside I see is that people might get confused that Filters.photo & Filters.regex will not do the expected thing, while Filters.photo & Filters.caption_regex does. But I guess I can live with that.

Maybe @Poolitzer has an opinion on that?

Nope that would be fine for me. We could mention it in Filters.photo doc but we dont do with text either so I think that would be the better solution.

The same as for regex, with only the content of the message changed, that is now inside caption.
Lines too long and an additional blank line
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 update! I take it that the tests are basically copy-pasted from Filters.regex, so those should be fine. Two points missing from what I see:

  1. docstring for the new filter (you can keep it shorter than the one of Filters.regex, just say "works the same" and link to Filters.regex)
  2. please make sure that pre-commit is happy. have a look at the contribution guide for how to set that up locally.

@marc0777
Copy link
Contributor Author

marc0777 commented Nov 4, 2020

Of course, next step is docstring!

I had some trouble configuring the pre-commit on my machine: it did succeed locally, yet the result was different. Now it should be fine (hopefully).

@Bibo-Joshi
Copy link
Member

I had some trouble configuring the pre-commit on my machine: it did succeed locally, yet the result was different. Now it should be fine (hopefully).

Maybe merge from master. We recently changed some of the config …

@marc0777
Copy link
Contributor Author

marc0777 commented Nov 4, 2020

Now everything should be fine, however merging from master made test-official fail, despite me not having touched anything that should trigger it.

Let me know what you think!

@marc0777 marc0777 requested a review from Bibo-Joshi November 4, 2020 17:27
@Bibo-Joshi
Copy link
Member

Yeah, TG just released API 5.0 today, so test official will be failing until we incorporate the changes … nothing to worry about. Looks good, I'll merg :) Thanks for the contribution!

@Bibo-Joshi Bibo-Joshi merged commit a0cd6e8 into python-telegram-bot:master Nov 4, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Nov 6, 2020
@Bibo-Joshi Bibo-Joshi added 🔌 enhancement pr description: enhancement and removed enhancement labels Nov 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

🔌 enhancement pr description: enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants