Skip to content

Conversation

@spontanurlaub
Copy link
Contributor

@spontanurlaub spontanurlaub commented Oct 21, 2017

Changes to PR #880 :

  • Added tests

  • Renamed Category Filter to category

  • Renamed FileSize Filter to file_size

  • Renamed FileType Filter to file_type

  • Removed mime_types from constants

  • Standard values at file_size Filter are None now and are only compared if a value is given -> no sys.maxsize

  • Usage of stdlib mimetypes, but sometimes types are missing or different from the mime_types sent by the official telegram app. Hardcoded the strings in this cases.

  • Changed the name attribute of category, file_size and file_type, so their attributes are shown

Still solves Issue #875

@codecov
Copy link

codecov bot commented Oct 21, 2017

Codecov Report

Merging #889 into master will decrease coverage by 0.14%.
The diff coverage is 80.32%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #889      +/-   ##
==========================================
- Coverage    91.7%   91.55%   -0.15%     
==========================================
  Files         101      101              
  Lines        4063     4123      +60     
  Branches      623      635      +12     
==========================================
+ Hits         3726     3775      +49     
- Misses        198      202       +4     
- Partials      139      146       +7
Flag Coverage Δ
#Appveyor 86.75% <80.32%> (-0.1%) ⬇️
#Travis 91.14% <80.32%> (-0.15%) ⬇️
Impacted Files Coverage Δ
telegram/ext/filters.py 95.08% <80.32%> (-4.03%) ⬇️
telegram/utils/request.py 67.85% <0%> (+0.89%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b3315d...a5f8f9b. Read the comment docs.

filesize = message.audio.file_size
elif message.document:
filesize = message.document.file_size
elif message.photo:
Copy link
Member

Choose a reason for hiding this comment

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

message.photo is an array of different photo sizes. Not sure if the filter even makes here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, thanks. Didn't thought about that. What about just using the photo in the highest resolution?

elif message.document:
filesize = message.document.file_size
elif message.photo:
filesize = message.photo.file_size
Copy link
Member

Choose a reason for hiding this comment

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

and this line will fail

@jh0ker
Copy link
Member

jh0ker commented Oct 21, 2017

Ideas for message.photo ?

"""This Filter filters documents by their category in the mime-type attribute
Note:
This Filter only filters by the mime_type of the document,
Copy link
Member

Choose a reason for hiding this comment

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

You didn't put an Args: section, and you should specifically mention that mime_type.startswith is used

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see you put the Args: in the __init__, sorry

of audio sent as file, for example "audio/mpeg" or "audio/x-wav"
"""

def __init__(self, category):
Copy link
Member

Choose a reason for hiding this comment

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

perhaps it would be clearer if we renamed the category parameter to mime_type_prefix

Filters.documents.file_type("audio/mpeg") filters all audio in mp3 format.
"""

def __init__(self, filetype):
Copy link
Member

Choose a reason for hiding this comment

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

and filetype to mime_type

Copy link
Contributor Author

@spontanurlaub spontanurlaub Oct 21, 2017

Choose a reason for hiding this comment

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

Yes, makes sense

video = category("video/")
text = category("text/")

class file_type(BaseFilter):
Copy link
Member

Choose a reason for hiding this comment

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

and perhaps even name the filter mime_type, to avoid confusion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I will change

@jh0ker
Copy link
Member

jh0ker commented Oct 21, 2017

So after some discussion with @code1mountain, we came to the conclusion it would make the most sense to allow any message where at least one of the different PhotoSizes is within the min and max limits.

Then in a second PR, we add helper methods to Message to get a specific PhotoSize within acceptable limits. It could look something like Message.get_photo_size(min_file_size=None, max_file_size=None, min_resolution=None, max_resolution=None, prefer_biggest=True) but nothing is set in stone yet.

Opinions?

@Eldinnie
Copy link
Member

Eldinnie commented Dec 5, 2017

What's the status on this?

@jh0ker
Copy link
Member

jh0ker commented Mar 16, 2018

closing in favor of #1046

@jh0ker jh0ker closed this Mar 16, 2018
@github-actions github-actions bot locked and limited conversation to collaborators Aug 21, 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.

3 participants