-
Notifications
You must be signed in to change notification settings - Fork 321
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
[pytx] Implement FileContent class #1680
base: main
Are you sure you want to change the base?
Changes from 1 commit
a661224
b31647e
8aff837
d18a8e9
95282f0
69d9abc
bb816aa
a5a1930
2580a48
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,30 @@ | ||||||||||
import typing as t | ||||||||||
from content_base import ContentType | ||||||||||
from photo import PhotoContent | ||||||||||
from video import VideoContent | ||||||||||
|
||||||||||
class FileContent(ContentType): | ||||||||||
""" | ||||||||||
Content representing a file. Determines if a file is a photo or video based on file extension. | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggested:
Suggested change
|
||||||||||
""" | ||||||||||
|
||||||||||
VALID_PHOTO_EXTENSIONS = {".jpg", ".jpeg", ".png", ".gif"} | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For photos, we can align with the image types that Pillow supports. Check out this code:
|
||||||||||
VALID_VIDEO_EXTENSIONS = {".mp4", ".avi", ".mov", ".mkv"} | ||||||||||
|
||||||||||
def __init__(self, file_name: str): | ||||||||||
self.file_name = file_name | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. blocking q: Currently, I don't any ContentType has an implementation for instantiation (we couldn't agree on what it should mean at the time, and so just saved it for the future). What would be your arguments to make file the first one? |
||||||||||
|
||||||||||
@classmethod | ||||||||||
def get_name(cls) -> str: | ||||||||||
return "File" | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I think this the same as a default implementation, and so can be removed |
||||||||||
|
||||||||||
def get_content_type_from_filename(self) -> t.Type[ContentType]: | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggested signature
Suggested change
|
||||||||||
""" | ||||||||||
Determines content type based on file extension. | ||||||||||
""" | ||||||||||
if any(self.file_name.endswith(ext) for ext in self.VALID_PHOTO_EXTENSIONS): | ||||||||||
return PhotoContent | ||||||||||
elif any(self.file_name.endswith(ext) for ext in self.VALID_VIDEO_EXTENSIONS): | ||||||||||
return VideoContent | ||||||||||
else: | ||||||||||
raise ValueError(f"Unknown content type for file: {self.file_name}") | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. blocking: Since the caller can't easily know whether the type is supported before calling, suggest making this return |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
import unittest | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. blocking: Please convert this to py.test as previously requested. Additionally, we want the tests for modules to live in the same directory as the code. Please move this file to threatexchange/content_type/tests/test_file_content_type.py |
||
from threatexchange.content_type.photo import PhotoContent | ||
from threatexchange.content_type.video import VideoContent | ||
from threatexchange.content_type.file_content import FileContent | ||
|
||
class TestFileContentType(unittest.TestCase): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. blocking: In this library, we use py.test and not unittest.TestCase. Take look at some of the other tests in this file to see how to write those. The logic overall looks good though! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can't find any tests that using py.test There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a very definitive statement! Here's one: https://github.com/facebook/ThreatExchange/blob/main/python-threatexchange/threatexchange/cli/tests/cli_smoke_test.py I will open issues for you to convert all of the old ones to py.test :) |
||
def test_photo_detection(self): | ||
file_content = FileContent("file.jpg") | ||
content_type = file_content.get_content_type_from_filename() | ||
self.assertEqual(content_type, PhotoContent) | ||
|
||
def test_video_detection(self): | ||
file_content = FileContent("file.mp4") | ||
content_type = file_content.get_content_type_from_filename() | ||
self.assertEqual(content_type, VideoContent) | ||
|
||
def test_unknown_file_type(self): | ||
file_content = FileContent("file.txt") | ||
with self.assertRaises(ValueError): | ||
file_content.get_content_type_from_filename() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blocking question: Walk me through your thoughts about these changes. Why are they on this PR?