feat: Adding exported value to extract_frames - #3390
Conversation
… use in downstream packages
C-Achard
left a comment
There was a problem hiding this comment.
Hello @sam-delap,
Thanks a lot for the contribution, we might adapt it a bit to retain full backwards compatibility, no need to implement the changes yourself, we are happy to handle it.
We will discuss this internally and take care of the PR once we agree on the best implementation.
Best,
Cyril
| extracted_cam=0, | ||
| videos_list=None, | ||
| ): | ||
| config: str, |
| @@ -173,7 +174,11 @@ def extract_frames( | |||
|
|
|||
| Returns | |||
There was a problem hiding this comment.
We may want to make returning the dict optional, so as to retain previous behavior where the success was reported (though the docstring return type was incorrect)
There was a problem hiding this comment.
Will make sure I update the docstring, apologies
| if all(has_failed): | ||
| print("Frame extraction failed. Video files must be corrupted.") | ||
| return has_failed | ||
| return frames_by_video |
There was a problem hiding this comment.
This will likely have to change slightly to retain backwards compatibility.
I would suggest we make the base return type bool/None to get granularity (failed/success with result/success with no result), and add a flag to request the frames dict as output in the signature, so it preserves the old behavior as default.
Or we can make an extra function and rely on a private underlying implementation to provide both without making the return type conditional.
There was a problem hiding this comment.
I'm OK gating this behind a flag... will update on my side accordingly
|
Merged with main to resolve some conflicts regarding the |
|
Hi @sam-delap thanks again for contributing. I am putting this PR in draft-status for now, as you are working on it. Let us know when you are finished adding your changes, then we will help out as well to get this in a production-ready state! 💪 |
Clearly document what changed and why
Added a consistent output type (and additional type hinting) to extract_frames function
Link any related issue(s)
Closes #3188
Update docstrings and documentation when behavior changes
Docstrings updated, from what I remember the API auto-generates the upstream docs here so no additional docs changes need to be made
Add or update tests when appropriate
extract_frames was not directly under test in any test suite components at present
Include a small usage example when it helps reviewers understand and/or test the change