-
Notifications
You must be signed in to change notification settings - Fork 155
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
New: add new ImagePost FeedItem model #330
base: master
Are you sure you want to change the base?
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
@@ -102,12 +127,11 @@ class FeedItem(BaseModel): | |||
music: MusicInfo | |||
stats: StatisticsInfo | |||
video: VideoInfo | |||
image_post: ImagePostlInfo = None |
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.
Optional?
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.
Sorry late response. I made it optional as it is not a property that is returned when it is a standard video.
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.
I now understand your comment.
Which way would you prefer?
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.
image_post: Optional[ImagePostlInfo]
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.
Thank you @Gr3gnov, I can not test it properly right now, I will try to push it tomorrow.
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.
Oh! And I will probably rename that class to ImagePostInfo.
@@ -80,8 +80,8 @@ class VideoInfo(BaseModel): | |||
duration: int | |||
ratio: str | |||
cover: HttpUrl | |||
play_addr: HttpUrl | |||
download_addr: HttpUrl | |||
play_addr: Optional[HttpUrl] |
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.
= None?
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.
So I assume you would keep this line as it is, right?
tiktokpy/models/feed.py
Outdated
fields: ClassVar[dict] = { | ||
"create_time": "createTime", | ||
} | ||
fields: ClassVar[dict] = {"create_time": "createTime", "image_post": "imagePost"} |
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.
fields: ClassVar[dict] = {"create_time": "createTime", "image_post": "imagePost"} | |
fields: ClassVar[dict] = { | |
"create_time": "createTime", | |
"image_post": "imagePost", | |
} |
Co-authored-by: Grigoriy Ivanov <[email protected]>
for more information, see https://pre-commit.ci
I was testing this code and looked promising. The model did not implement ImagePost as a FeedItem, I included the basic classes so that both videos and imagePosts can be successfully handled by the model.
I have not dived in at all, the changes are basically:
Added a new property to the existing FeedItem class, when the item is a regular video, you do not receive the new property, so it is initialized to None.
When the item is ImagePost, it still has video property, but two required properties will be missing, so I made them Optional in the VideoInfo definition:
Lastly I realized that in the new imagePost data, properties with 'image type' values had a consistent structure (imagePost cover, shareCover and images). So I added imagePostInfo model and a new class for these properties: