Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a standalone schema module for FFmpeg options, refactors existing parsing code to use it, and updates code generation to build options via the new schema.
- Added
schema.pydefiningFFMpegOption,FFMpegOptionType, andFFMpegOptionFlag. - Updated imports in parsing utilities to reference the new schema.
- Enhanced
load_optionsin code generation to constructFFMpegOptionobjects using the new enums.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/scripts/parse_c/schema.py | New schema definitions for FFmpeg options (types, flags, dataclass) |
| src/scripts/parse_c/parse_ffmpeg_opt_c.py | Refactored imports to use .schema for FFMpegOption and flags |
| src/scripts/parse_c/cli.py | Updated import to .schema (unused import may be removable) |
| src/scripts/code_gen/cli.py | Improved load_options to rebuild options with the new schema |
Comments suppressed due to low confidence (2)
src/scripts/parse_c/schema.py:185
- [nitpick] The property name
is_support_stream_specifieris a bit awkward; consider renaming it tosupports_stream_specifierfor clarity and consistency with boolean naming conventions.
def is_support_stream_specifier(self) -> bool:
src/scripts/parse_c/cli.py:16
FFMpegOptionis imported but not used in this module; consider removing the unused import to reduce clutter and dependencies.
from .schema import FFMpegOption
| options = [ | ||
| FFMpegOption( | ||
| name=i.name, | ||
| type=FFMpegOptionType(i.type.value), |
There was a problem hiding this comment.
You can directly assign the enum instance (type=i.type) instead of re-wrapping it with FFMpegOptionType(i.type.value), simplifying the code and avoiding redundant conversion.
| type=FFMpegOptionType(i.type.value), | |
| type=i.type, |
|
|
||
| options = parse_c.cli.parse_ffmpeg_options() | ||
| options = [ | ||
| FFMpegOption( |
There was a problem hiding this comment.
When recreating FFMpegOption instances, optional fields like argname and canon are omitted; include argname=i.argname and canon=i.canon to preserve these values.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #764 +/- ##
==========================================
+ Coverage 80.89% 87.98% +7.09%
==========================================
Files 77 27 -50
Lines 4469 1215 -3254
==========================================
- Hits 3615 1069 -2546
+ Misses 854 146 -708
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
This pull request introduces significant changes to improve the handling and representation of FFmpeg command-line options in the codebase. The most important updates include the addition of a new schema module defining
FFMpegOptionand related enums, refactoring imports to use the new schema, and enhancing theload_optionsfunction to utilize the updatedFFMpegOptionstructure.Schema Enhancements:
src/scripts/parse_c/schema.py: Added a new schema module definingFFMpegOption,FFMpegOptionType, andFFMpegOptionFlag. This provides a structured and extensible way to represent FFmpeg options, including properties for input/output/global options and stream specifier support.Refactoring for Schema Integration:
src/scripts/parse_c/cli.py: Updated imports to use the newschema.pymodule forFFMpegOption, replacing the previous import fromffmpeg.common.schema.src/scripts/parse_c/parse_ffmpeg_opt_c.py: Refactored imports to useschema.pyforFFMpegOptionandFFMpegOptionFlag.Functional Improvements:
src/scripts/code_gen/cli.py: Enhanced theload_optionsfunction to constructFFMpegOptionobjects using the newFFMpegOptionTypeenum and other schema properties, providing better type safety and clarity.Minor Updates:
src/scripts/code_gen/cli.py: AddedFFMpegOptionTypeto the list of imports for schema integration.