Adding Filters for mime-types and file_size #880
Conversation
Codecov Report
@@ Coverage Diff @@
## master #880 +/- ##
==========================================
+ Coverage 91.31% 91.38% +0.06%
==========================================
Files 101 101
Lines 4055 4132 +77
Branches 620 628 +8
==========================================
+ Hits 3703 3776 +73
- Misses 204 217 +13
+ Partials 148 139 -9
Continue to review full report at Codecov.
|
jsmnbom
left a comment
jsmnbom
left a comment
There was a problem hiding this comment.
Preliminary review.
Mostly looks good, seems like useful filters to have in many situations :D
I have a few comments I'd like you to review :)
| text = "text/" | ||
|
|
||
|
|
||
| class Document: |
There was a problem hiding this comment.
Is there a good reason you're not using the stdlib mimetype?
There was a problem hiding this comment.
No, just didn't found them. I will fix this.
There was a problem hiding this comment.
The stdlib mimetype doesn't work for many mimetypes in telegram. ".exe" documents for example have the mimetype "application/x-ms-dos-executable" if you send it with an official telegram app, but ".exe" is matched to "application/octet-stream" in the stdlib mimetype.
| if message.document: | ||
| return bool(message.document.mime_type.startswith(self.category)) | ||
|
|
||
| application = Category(constants.MediaCategory.application) |
There was a problem hiding this comment.
Could we perhaps expose these as filters.document.category.application instead of just filters.document.application?
There was a problem hiding this comment.
@bomjacob
is this nesting really necessary? I think that filters.document.application is much more easier to use.
| if message.document: | ||
| return bool(message.document.mime_type == self.filetype) | ||
|
|
||
| apk = FileType(constants.Document.apk) |
There was a problem hiding this comment.
Could we perhaps expose these as filters.document.filetype.apk instead of just filters.document.apk?
There was a problem hiding this comment.
It's much more nesting. Not sure we really need this.
| """ | ||
| name = "Filters.FileSize()" | ||
|
|
||
| def __init__(self, min=0, max=sys.maxsize): |
There was a problem hiding this comment.
I'm not a huge fan of using sys.maxsize. Could you instead have it default to None and if it is None just don't compare upper limit?
| self.max = max | ||
|
|
||
| def filter(self, message): | ||
| if bool(message.audio): |
There was a problem hiding this comment.
There's no need for the bool() in these if statements
There was a problem hiding this comment.
Ok, I will leave it
| Examples: | ||
| Filters.documents.FileType("audio/mpeg") filters all audio in mp3 format. | ||
| """ | ||
| name = "Filters.document.Filetype()" |
There was a problem hiding this comment.
I will fix it
| document = _Document() | ||
| """:obj:`Filter`: Messages that contain :class:`telegram.Document`.""" | ||
|
|
||
| class FileSize(BaseFilter): |
| class FileSize(BaseFilter): | ||
| """This Filter filters all messages with a `file_size` attribute. | ||
| """ | ||
| name = "Filters.FileSize()" |
| filesize = message.voice.file_size | ||
| else: | ||
| return False | ||
| return bool(self.min <= filesize <= self.max) |
There was a problem hiding this comment.
I will leave it too. Fixed PR will come tomorrow. Thank you for your feedback.
| MAX_INLINE_QUERY_RESULTS = 50 | ||
|
|
||
|
|
||
| class MediaCategory: |
There was a problem hiding this comment.
Just put these constants in the filters.py, since that's the only place they are used and they aren't meant to be exported to the general user.
There was a problem hiding this comment.
Not necessary if I use built-in mimetypes
|
Does #889 mean this one should be closed, @code1mountain ? |
|
@code1mountain please respond |
|
@code1mountain we're gonna go ahead and close this fairly soon if we receive no reply. It's been pending reply for almost 2 months now. Just give us a heads up if you're still working on it ^^ |
|
closing as of no response from the author of the PR |
Bibo-Joshi
added
🔌 enhancement
6 participants
Adding new subfilters to Filters.documents that allow to filter for different mime_type categories or file types.
Adding new Category Filter for these file categories:
application/
audio/
image/
video/
text/
Adding Filters for these file types:
apk, docx, exe, gif, jpg, mp3, pdf, png, py, svg, txt, targz, wav, xml, zip
Adding Filters.FileSize(min=0, max=sys.maxsize) to filter all types of media by it's file_size.
The Category Filter and the FileType Filter are visible, because it should be possible to create own Filters for other mime_types.
This solves Issue #875
Contact for questions: t.me/bergfreak