chore: low-priority cleanups (logging, imports, deps, atempo, test hygiene) - #8
Open
rsmxingu wants to merge 1 commit into
Open
chore: low-priority cleanups (logging, imports, deps, atempo, test hygiene)#8rsmxingu wants to merge 1 commit into
rsmxingu wants to merge 1 commit into
Conversation
…giene)
Bundle of the low-priority review items:
- Logging: replace the manual remove/truncate-on-startup dance with
RotatingFileHandler(mode="w"). Each session still starts with a fresh log
(documented behavior preserved) and the rotating handler still caps in-session
growth — one mechanism instead of two overlapping ones.
- ui.py: hoist `import sys` / `import ctypes` from inside __init__ to module top.
- processor.py: add _atempo_chain() so playback speeds outside ffmpeg's
single-filter [0.5, 2.0] atempo range are decomposed into a valid chain
(e.g. 2.5 -> "atempo=2.0000,atempo=1.2500"). The common 1.2x case is
unchanged ("atempo=1.2000"). Fixes a latent failure for clips auto-editor
marks faster than 2x. Used in both the cut and no-cut audio paths.
- requirements: add conservative upper bounds; pin auto-editor <30 since the
frozen-bundle integration depends on v29 internals. Add requirements-dev.txt
(pytest) for the test suite.
- Rename test_pipeline.py -> integration_pipeline.py: it is a manual,
ffmpeg-driven smoke script, not a unit test; the test_*.py name made pytest
collect and execute the whole pipeline on a normal test run.
Tests:
- Add TestAtempoChain (in-range, >2.0 chaining, far->chained, <0.5 chaining).
- Fix a stale progress-math comment in test_run_success_two_pass.
- tests/test_processor: 12 passed.
Deferred: silence-threshold input validation pairs with the messagebox import
fix (separate PR) and is intentionally left out here to avoid coupling.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bundles the low-priority review items into one PR. No user-facing behavior changes except the atempo edge-case fix.
Changes
Logging — one mechanism instead of two
_setup_logging()did a manualos.remove/truncate(0)dance and used aRotatingFileHandler. Replaced withRotatingFileHandler(mode="w"): each session still starts with a fresh log (the behavior documented in the README) and the rotating handler still caps in-session growth.ui.py — hoist imports
import sys/import ctypesmoved from inside__init__to the module top.processor.py — atempo chaining for extreme speeds
A single ffmpeg
atempoonly accepts factors in[0.5, 2.0]. Added_atempo_chain()which decomposes out-of-range speeds into a valid chain (e.g.2.5 -> "atempo=2.0000,atempo=1.2500"). The common 1.2x path is byte-for-byte unchanged. This fixes a latent failure when auto-editor marks a clip faster than 2x. Applied to both the cut and no-cut audio paths.requirements
auto-editor<30because the frozen-bundle integration relies on v29 internals (.v3timeline,--no-seek,download_binaryhook).requirements-dev.txt(pytest).Test hygiene
test_pipeline.py→integration_pipeline.py. It is a manual, ffmpeg-driven smoke script, not a unit test — thetest_*.pyname caused pytest to collect and run the whole pipeline on a normal test run.TestAtempoChain(4 cases).test_run_success_two_pass.Deferred
Silence-threshold input validation was intentionally left out: it depends on
messagebox, which is fixed in a separate PR ("import tkinter.messagebox explicitly"). Adding it here would couple the two.Testing
python3 -m py_compile processor.py ui.py tests/test_processor.py integration_pipeline.pypasses.python3 -m unittest tests.test_processor→ 12 passed.tests/test_ui.pyrequires tkinter (unavailable in my sandbox); please runpython3 -m unittest tests.test_uito confirm the ui.py import hoist there.🤖 Generated with Claude Code