[output] Support paths and file handles in scene-list writers - #570
[output] Support paths and file handles in scene-list writers#570MuhammadBilalKhan267 wants to merge 3 commits into
Conversation
Breakthrough
left a comment
Breakthrough
left a comment
There was a problem hiding this comment.
Thanks for getting started with this one, there's a lot of moving parts here.
| if not isinstance(output_file, (str, Path)): | ||
| return func(*args, **kwargs) | ||
|
|
||
| with open(output_file, "w", newline=newline) as file_handle: |
There was a problem hiding this comment.
This will open and truncate the file before the writer operates, which means any exceptions during generation leave an empty file or clobber an existing one.
Can we render the contents of the file in memory first, only opening + writing it to disk once the body is constructed? Most writers build the whole document in memory so this should be relatively cheap.
There was a problem hiding this comment.
I had noticed this earlier but followed the pattern introduced in #566, where a path is opened in write mode before delegating to the writer. Changed now.
| def wrapper(*args: ty.Any, **kwargs: ty.Any): | ||
| output_file = args[0] if args else kwargs[parameter_name] | ||
|
|
||
| if not isinstance(output_file, (str, Path)): |
There was a problem hiding this comment.
This doesn't necessarily work if the OS uses bytes for paths. Previously we coerced the input into Path(output_path) to ensure the type - consider using isinstance(output_file, (str, bytes, os.PathLike)) here and annotating the writers as StrPath | ty.TextIO.
| if not isinstance(output_file, (str, Path)): | ||
| return func(*args, **kwargs) | ||
|
|
||
| with open(output_file, "w", newline=newline) as file_handle: |
There was a problem hiding this comment.
I think we need to add an explicit encoding here, UTF-8 seems like a good candidate (set encoding="utf-8" here).
| return decorator | ||
|
|
||
|
|
||
| @_open_output_file(parameter_name="output_csv_file", newline="") |
There was a problem hiding this comment.
The CSV writer passes newline="" but the other four open with the default value, so on Windows the same writer could emit CRLF when given a path, but then LF when given a handle.
Path-mode CRLF was pre-existing, but we should make the behaviour consistent. Since every writer emits its own line endings, maybe we could just open with newline="" unconditionally in the decorator and drop newline for this special case. Thoughts?
| def decorator(func: _F) -> _F: | ||
| @functools.wraps(func) | ||
| def wrapper(*args: ty.Any, **kwargs: ty.Any): | ||
| output_file = args[0] if args else kwargs[parameter_name] |
There was a problem hiding this comment.
Can we use a single checked source of truth for the filename somehow? Or just drop the parameter name entirely and assert at decoration time that the first param is the file always?
There was a problem hiding this comment.
Done. Now we get the name of the file parameter from the function signature.
| row_separator=row_separator, | ||
| ) | ||
| return | ||
| output_csv_file = ty.cast(ty.TextIO, output_csv_file) |
There was a problem hiding this comment.
Can we just assert the instance is of type X instead of casting here and elsewhere? (e.g .assert not isinstance(output_path, (str, os.PathLike)))
This avoids type errors when someone passes in the wrong type.
Also: some writers reassign the arg in place, while the other four introduce a new output_file name. Might be worth consistency here to guide a future API change, since I do think the current arg name is too verbose. That's for another time though of course.
There was a problem hiding this comment.
The decorator converts paths to StringIO before calling the writer and rejects objects without write(). The suggested assertion would therefore always pass. ty.cast() only tells Pyright that this is a text file.
There was a problem hiding this comment.
Assertions about meta attributes should also fulfill Pyright's resolution. The reason I prefer the assertion is that it ensures we fail loudly if for whatever reason someone tries to change it incorrectly.
| # `scenedetect` is imported lazily to avoid a circular import at module load. | ||
| import scenedetect | ||
| logger.info( | ||
| "Writing scenes in EDL format to %s", |
There was a problem hiding this comment.
We do the getattr thing a lot here, can we just add this to the decorator to avoid repeating it?
| _F = ty.TypeVar("_F", bound=ty.Callable[..., ty.Any]) | ||
|
|
||
|
|
||
| def _open_output_file(parameter_name: str, *, newline: str | None = None): |
There was a problem hiding this comment.
Nit: consider adding a return type annotation here and for the inner wrapper for consistency with the other definitions in this file.
| assert "TITLE:" in output_path.read_text() | ||
|
|
||
|
|
||
| def test_write_scene_list_edl_accepts_path(tmp_path: Path): |
There was a problem hiding this comment.
The four new *_accepts_path tests seem to be subsets of existing tests directly above them. The remaining eight could also be parameterized to avoid so much code duplication. Consider deleting the _accepts_path tests and instead parameterize over the target kind (file handle, string, os.Path, etc). See other uses of @pytest.mark.parametrize in this file for examples.
There was a problem hiding this comment.
I initially followed the separate file-handle/string/Path test pattern from #566. Changed now.
2 participants
Closes #567.
str,Path, and open text files for EDL, FCPXML, FCP7 XML, and OTIO output.Tests: 382 passed, 2 skipped.