Skip to content

Accept a path or file handle in write_scene_list - #566

Closed
jennifferweslowski-design wants to merge 3 commits into
Breakthrough:mainfrom
jennifferweslowski-design:fix-523-write-scene-list-path
Closed

Accept a path or file handle in write_scene_list#566
jennifferweslowski-design wants to merge 3 commits into
Breakthrough:mainfrom
jennifferweslowski-design:fix-523-write-scene-list-path

Conversation

@jennifferweslowski-design

Copy link
Copy Markdown

Fixes #523

write_scene_list("scenes.csv", scenes) currently raises TypeError: argument 1 must have a "write" method because the path string is passed straight to csv.writer.

This change lets the first argument be a file handle (existing callers) or a str / pathlib.Path. A path is opened with newline="" inside a context manager so the file is closed after writing, which matches the concern on the issue.

Tests cover a StringIO handle, a str path, and a Path. No video fixtures.

I could not run the suite in this environment; the new tests follow the existing _fake_scenes pattern in tests/test_output.py.

Open path arguments with a context manager so the file is closed after writing. Existing file-handle callers are unchanged.

@Breakthrough Breakthrough left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Needs formatting w/ ruff but LGTM otherwise! 👍

@jennifferweslowski-design

Copy link
Copy Markdown
Author

After further review of the discussion in #523, I realize this PR takes the opposite direction from the maintainer guidance that callers should pass an already-opened file handle (and use a context manager themselves).

Even though the change is test-covered and received approval, pushing an API that conflicts with the stated design intent is not the right outcome. I'm closing this PR to avoid further review time and noise.

Sorry for the confusion, and thank you for the careful feedback.

@Breakthrough

Copy link
Copy Markdown
Owner

Consider keeping this open, there is some existing precedent for allowing this in other libraries.

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.

write_scene_list(csv_file)

2 participants