Skip to content
This repository was archived by the owner on Oct 23, 2019. It is now read-only.

MAX_SIZE minor bug fix #99

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

MAX_SIZE minor bug fix #99

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
abe7caf
bug fix, MAXSIZE defined in config did not work cause there were no c…
parasteh Dec 5, 2018
b40ec53
Add .idea to the list
parasteh Dec 5, 2018
716c248
remove .idea from the remote repo
parasteh Dec 5, 2018
874af06
2 correction, first file_size is now working, it get compared with ma…
parasteh Dec 6, 2018
f2869c3
undo changes in gitignore file
parasteh Dec 6, 2018
0b0e3c7
Better formatting
Lonami Dec 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,9 @@ target/
# pyenv
.python-version

#pycharm
.idea
Copy link
Collaborator

@Lonami Lonami Dec 5, 2018

Choose a reason for hiding this comment

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

This belongs in your global .gitignore, so please undo this change.


# celery beat schedule file
celerybeat-schedule

Expand Down
3 changes: 2 additions & 1 deletion telegram_export/downloader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@ def _check_media(self, media):
"""
Checks whether the given MessageMedia should be downloaded or not.
"""
if not media or not self.max_size:
# It is needed to chek the size with the max_size defined in config file
if not media or media.document.size > self.max_size:
Copy link
Collaborator

@Lonami Lonami Dec 5, 2018

Choose a reason for hiding this comment

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

Did you check at least twice the Telethon documentation to make sure document is not None and size is not None (or they can't be None, not present at all)? We don't want an error here.

Copy link
Author

@parasteh parasteh Dec 5, 2018

Choose a reason for hiding this comment

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

I will fix it, let you know the result

Copy link
Author

@parasteh parasteh Dec 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

There are some points need consideration, Media has multiple types and it is not easy to get the size of each object, I mean it is depended to the Type and calling media.document.size will only work for document type and no other. I think we need to use polymorphism to do that. each type has to implement an interface for general attributes like size, so calling something like media.getsize() return the size of the file regardless of the type of it,

Copy link
Collaborator

@Lonami Lonami Dec 6, 2018

Choose a reason for hiding this comment

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

each type has to implement an interface for general attributes like size

More like some utils method get_size(media), unless you're willing to do this change in Telethon which would be by far harder :)

Copy link
Author

@parasteh parasteh Dec 6, 2018

Choose a reason for hiding this comment

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

yes, I found an easier way to do that, but not sure yet, so far I realized that just document types have size attribute, so when calling export_utils.get_media_type(media) returns "document" we can refer to media.document.size and compare it to the max_size,
What's your idea?

Copy link
Author

@parasteh parasteh Dec 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

also in utils there is a method named get_file_location(media):, So already it has been implemented, I will use it and after test I will commit the code

Copy link
Collaborator

@Lonami Lonami Dec 6, 2018

Choose a reason for hiding this comment

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

Just add another function to utils.py to get_file_size for media, comparing media type with isinstance.

Copy link
Author

@parasteh parasteh Dec 6, 2018

Choose a reason for hiding this comment

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

its done !, how can I pull the changes ?

return False
if not self.types:
return True
Copy link
Collaborator

@Lonami Lonami Dec 7, 2018

Choose a reason for hiding this comment

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

This bypasses the max_size check. Are you sure this is the expected behaviour?

Copy link
Author

@parasteh parasteh Dec 8, 2018

Choose a reason for hiding this comment

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

Yes it is if you check the config file, it says "Setting to "0" will not download any media."
So I think there is no problem with this one

Copy link
Collaborator

@Lonami Lonami Dec 8, 2018

Choose a reason for hiding this comment

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

No, that's not what I meant. I mean, if there are no types (i.e. "all media is valid"), the maximum size is not checked at all.

Copy link
Author

@parasteh parasteh Dec 8, 2018

Choose a reason for hiding this comment

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

That's right, So the file size should be checked before this line!

Expand Down