Skip to content
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

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

Conversation

jbelinchonm
Copy link

@jbelinchonm jbelinchonm commented Oct 19, 2023

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.

class FeedItem(BaseModel):
    id: str
    ...
    video: VideoInfo
+    image_post: ImagePostlInfo = None
    challenges: List[ChallengeInfo] = []

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:

class VideoInfo(BaseModel):
    id: str
    ...
    cover: HttpUrl
-  play_addr: HttpUrl
-  download_addr: HttpUrl
+  play_addr: Optional[HttpUrl]
+  download_addr: Optional[HttpUrl]

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:

+ class ImageUrls(BaseModel):
+     url_list: List[HttpUrl]
+ 
+     class Config:
+         fields: ClassVar[dict] = {"url_list": "urlList"}
+ 
+ 
+ class ImageInfo(BaseModel):
+     image_height: int
+     image_width: int
+     image_url: ImageUrls
+ 
+     class Config:
+         fields: ClassVar[dict] = {
+             "image_url": "imageURL",
+             "image_width": "imageWidth",
+             "image_height": "imageHeight",
+         }
+ 
+ 
+ class ImagePostlInfo(BaseModel):
+     cover: ImageInfo
+     images: List[ImageInfo]

@@ -102,12 +127,11 @@ class FeedItem(BaseModel):
music: MusicInfo
stats: StatisticsInfo
video: VideoInfo
image_post: ImagePostlInfo = None
Copy link
Collaborator

Choose a reason for hiding this comment

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

Optional?

Copy link
Author

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.

Copy link
Author

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

image_post: Optional[ImagePostlInfo]

Copy link
Author

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.

Copy link
Author

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.

@sudoguy sudoguy requested a review from Gr3gnov October 29, 2023 18:47
@@ -80,8 +80,8 @@ class VideoInfo(BaseModel):
duration: int
ratio: str
cover: HttpUrl
play_addr: HttpUrl
download_addr: HttpUrl
play_addr: Optional[HttpUrl]
Copy link
Collaborator

Choose a reason for hiding this comment

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

= None?

Copy link
Author

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?

fields: ClassVar[dict] = {
"create_time": "createTime",
}
fields: ClassVar[dict] = {"create_time": "createTime", "image_post": "imagePost"}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
fields: ClassVar[dict] = {"create_time": "createTime", "image_post": "imagePost"}
fields: ClassVar[dict] = {
"create_time": "createTime",
"image_post": "imagePost",
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants