Skip to content

Improve config handling and add root tests#63

Open
mxl wants to merge 3 commits into
byjlw:mainfrom
mxl:feat/config-and-testing-improvements
Open

Improve config handling and add root tests#63
mxl wants to merge 3 commits into
byjlw:mainfrom
mxl:feat/config-and-testing-improvements

Conversation

@mxl

@mxl mxl commented Apr 7, 2026

Copy link
Copy Markdown
Contributor

Summary

  • fix config merging so user config overrides are applied on top of packaged defaults
  • make --config accept a user config JSON file path and clarify that behavior in the docs
  • fix CLI/config precedence bugs for --output, --keep-frames, and max_frames
  • add a root pytest setup and regression coverage for prompt loading and config behavior
  • add a dev extra so test dependencies can be installed with pip install \".[dev]\"

Details

Config loading and CLI behavior

  • load packaged default_config.json first, then deep-merge the user config on top
  • ensure missing top-level sections like clients and audio are initialized before CLI overrides are applied
  • map --output to output_dir, which is the config key the CLI actually uses
  • change --keep-frames handling so omitting the flag no longer overwrites keep_frames from user config
  • add config-file support for frames.max_frames
  • resolve max_frames with the expected precedence:
    • CLI --max-frames
    • config frames.max_frames
    • fallback sys.maxsize

Prompt loading

  • make PromptLoader prefer a user-specified prompt directory over packaged prompts so custom prompts can actually override defaults

Test setup

  • replace the ad hoc root test script with a standard pytest layout
  • add pytest.ini
  • add root tests for:
    • prompt loading from packaged resources
    • prompt loading from a custom prompt directory
    • missing prompt name handling
    • config merge semantics
    • --output mapping
    • --keep-frames precedence
    • max_frames config support and CLI precedence

Packaging and contributor workflow

  • add a dev extra in setup.py:
    • pip install \".[dev]\"
  • document the dev install and test workflow in README.md and docs/CONTRIBUTING.md

Documentation

  • update docs/USAGES.md to match current behavior
  • document missing supported config keys such as:
    • prompts
    • audio.whisper_model
    • audio.device
  • clarify that --config points to a user config JSON file
  • clarify --max-frames default as sys.maxsize (effectively no limit)

Testing

  • python -m pytest -q

Result

  • root test suite now exists and passes
  • config precedence is closer to the documented cascade model
  • custom prompt overrides work as expected

@github-actions

Copy link
Copy Markdown

🤖 Automated First-Pass Review

Model: qwen/qwen3-235b-a22b via OpenRouter. A human maintainer will follow up.


Summary

This PR improves configuration handling by deep-merging user config with defaults, adjusts CLI argument behavior, restructures tests into a pytest framework, enhances prompt loading, and updates documentation. New tests ensure correctness of config merging, CLI flags, and prompt loading.

Recommendation

📝 REQUEST_CHANGES — The PR introduces a new dependency 'audioop-lts' without justification and changes the '--config' CLI behavior in a backwards-incompatible way.

Comment thread docs/USAGES.md

## Configuration System

The tool uses a cascading configuration system with the following priority:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Changed '--config' behavior from expecting a directory in v0 to a JSON file in this PR is a breaking change. Old interface must be deprecated with warning or documented in migration notes, not just changed unconditionally.

Comment thread .gitignore

.DS_Store
.aider*

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

User config path '.gitignore' entry should match the new default config path used in the PR ('video_analyzer/config/config.json') rather than referencing the entire config directory.

Comment thread tests/test_config.py
args = argparse.Namespace(
video_path="video.mp4",
config=str(config_path),
output=None,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The test for resolve_max_frames includes no validation that the CLI parameter works correctly with a negative value - this could silently produce invalid output. Should explicitly test min(1, args.max_frames) or similar behavior.

@byjlw

byjlw commented Apr 19, 2026

Copy link
Copy Markdown
Owner

Thanks for contributing.
The PR introduces a new dependency 'audioop-lts' without justification and changes the '--config' CLI behavior in a backwards-incompatible way.

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