Skip to content

Fix save-html --no-images ignoring -o - #583

Merged
Breakthrough merged 5 commits into
Breakthrough:mainfrom
vishal-tambi:fix/save-html-test-output
Oct 22, 2026
Merged

Breakthrough merged 5 commits into
Breakthrough:mainfrom
vishal-tambi:fix/save-html-test-output

Conversation

@vishal-tambi

@vishal-tambi vishal-tambi commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

save-html --no-images ignored the global -o and wrote the HTML to the working directory, which is why test_cli_save_html left goldeneye-Scenes.html in the repo root. CliContext.save_images_result started as (None, None) instead of None, so save_html took its output folder from a save-images run that never happened. It now starts as None, and the test checks that the file lands in the -o folder.

@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.

with --no-images, save-html ignores -o. CliContext.save_images_result starts as (None, None) instead of None, so save_html takes its output folder from a save-images run that never happened. I can fix that in a separate PR if you'd like.

Thanks for kicking this off. We should change that behaviour to fix this properly here. By -o do you mean the global or local directory option (or both)?

@vishal-tambi

Copy link
Copy Markdown
Contributor Author

Just the global -o, since save-html doesn't have a local one. Fixed it here: save_images_result now starts as None, so --no-images writes to -o, and the test checks that.

@vishal-tambi vishal-tambi changed the title Stop test_cli_save_html from writing into the repo root Fix save-html --no-images ignoring -o Oct 20, 2026
@Breakthrough

Copy link
Copy Markdown
Owner

Just the global -o, since save-html doesn't have a local one. Fixed it here: save_images_result now starts as None, so --no-images writes to -o, and the test checks that.

Should we add one as well?

@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.

LGTM, will merge after you take a look at last set of comments/questions. Thanks for the PR!

Comment thread scenedetect/_cli/context.py Outdated
@vishal-tambi

vishal-tambi commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor Author

Should we add one as well?

I'd do that in a separate PR, and I can open it after this one merges. The HTML links to images with paths relative to the save-images folder, so a local -o would need those links fixed too.

thanks

@Breakthrough
Breakthrough merged commit faa3d97 into Breakthrough:main Oct 22, 2026
27 checks passed
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.

2 participants