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

Movie storage #545

Merged
merged 12 commits into from
Mar 19, 2024
Merged

Movie storage #545

merged 12 commits into from
Mar 19, 2024

Conversation

david-zwicker
Copy link
Member

A first version of a special storage class that stores field in a movie using ffmpeg

Comment on lines 40 to 41
def pix_fmt_data(self) -> str:
return {1: "gray", 3: "rgb24", 4: "rgba"}[self.channels]

Choose a reason for hiding this comment

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

Suggested change
def pix_fmt_data(self) -> str:
return {1: "gray", 3: "rgb24", 4: "rgba"}[self.channels]
def pix_fmt_data(self) -> str:
if self.channels == 1:
return "gray"
elif self.channels == 3:
return "rgb24"
elif self.channels == 4:
return "rgba"
else:
raise ValueError()

In this way it is much easier to understand what is going on.

Moreover, what happens if self.channels is not one of 1, 3, 4 ?

With your method: a key-error will be thrown but actually is a value error because the class work just if we have self.channels in [1, 3, 4].

Perhaps, an idea could be to add a check in the post_init method to ensure that channels is one of the above one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point! I'll adjust it to throw a more useful error in the future.

@david-zwicker david-zwicker merged commit 23095ea into master Mar 19, 2024
16 checks passed
@david-zwicker david-zwicker deleted the movie_storage branch March 19, 2024 14:25
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.

None yet

2 participants