From 73f1c00e74fa2a59d7e0df6c21efe772bde58314 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 15 Feb 2026 17:27:05 +0000 Subject: [PATCH] Fix code review issues: circular imports, type hints, cross-platform paths, test isolation Co-authored-by: Askill <16598120+Askill@users.noreply.github.com> --- Application/Config.py | 9 +++++++-- Application/VideoReader.py | 2 +- Application/__init__.py | 6 ++++-- docker-compose.yml | 2 -- main.py | 7 ++++--- pyproject.toml | 2 ++ tests/test_config.py | 12 +++++------- tests/test_logger.py | 19 +++++++++++++------ 8 files changed, 36 insertions(+), 23 deletions(-) diff --git a/Application/Config.py b/Application/Config.py index c6af5d5..76a6150 100644 --- a/Application/Config.py +++ b/Application/Config.py @@ -11,9 +11,12 @@ try: except ImportError: YAML_AVAILABLE = False -from Application.Logger import get_logger -logger = get_logger(__name__) +def _get_logger(): + """Lazy load logger to avoid circular imports.""" + from Application.Logger import get_logger + + return get_logger(__name__) class Config: @@ -50,6 +53,7 @@ class Config: If None or invalid, uses defaults. Supports .json, .yaml, and .yml extensions. """ + logger = _get_logger() if config_path and os.path.isfile(config_path): logger.info(f"Using supplied configuration at {config_path}") try: @@ -104,6 +108,7 @@ class Config: def _apply_env_overrides(self): """Apply environment variable overrides to configuration.""" + logger = _get_logger() env_prefix = "VIDEO_SUMMARY_" for key in self.c.keys(): env_key = f"{env_prefix}{key.upper()}" diff --git a/Application/VideoReader.py b/Application/VideoReader.py index de344c7..2405c33 100644 --- a/Application/VideoReader.py +++ b/Application/VideoReader.py @@ -76,7 +76,7 @@ class VideoReader: """Stop the video reading thread and wait for it to complete.""" self.thread.join() - def pop(self) -> Tuple[int, Optional[any]]: + def pop(self) -> Tuple[int, Optional["np.ndarray"]]: """ Pop next frame from buffer. diff --git a/Application/__init__.py b/Application/__init__.py index 880dc86..8270256 100644 --- a/Application/__init__.py +++ b/Application/__init__.py @@ -27,7 +27,10 @@ try: except ImportError as e: import warnings - warnings.warn(f"Some video processing components could not be imported: {e}") + warnings.warn( + f"Video processing components could not be imported. Missing dependency: {e.name if hasattr(e, 'name') else str(e)}. " + f"Install with: pip install -r requirements.txt" + ) # Try to import LayerManager (may require TensorFlow for classification features) try: @@ -38,4 +41,3 @@ except ImportError: import warnings warnings.warn("LayerManager could not be imported. TensorFlow may be required for classification features.") - diff --git a/docker-compose.yml b/docker-compose.yml index dead2be..b8eddc0 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -1,5 +1,3 @@ -version: '3.8' - services: video-summary: build: . diff --git a/main.py b/main.py index c4bbeb8..b3d4570 100644 --- a/main.py +++ b/main.py @@ -1,4 +1,5 @@ import argparse +import logging import os import sys import time @@ -113,7 +114,7 @@ For more information, see: https://github.com/Askill/Video-Summary # Setup logging level if args.verbose: - setup_logger(level=10) # DEBUG level + setup_logger(level=logging.DEBUG) try: # Load configuration @@ -131,12 +132,12 @@ For more information, see: https://github.com/Askill/Video-Summary # Create output directory if it doesn't exist os.makedirs(output_path, exist_ok=True) - file_name = input_path.split("/")[-1] + file_name = os.path.basename(input_path) # Configure paths config["inputPath"] = input_path config["outputPath"] = os.path.join(output_path, file_name) - config["cachePath"] = os.path.join(output_path, file_name.split(".")[0]) + config["cachePath"] = os.path.join(output_path, os.path.splitext(file_name)[0]) # Get video dimensions logger.info("Reading video dimensions...") diff --git a/pyproject.toml b/pyproject.toml index 400cf2c..d9a8e99 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -34,6 +34,8 @@ dependencies = [ "imageio>=2.9.0", "imageio-ffmpeg>=0.4.0", "matplotlib>=3.3.0", + "pyyaml>=6.0", + "tqdm>=4.60.0", ] [project.optional-dependencies] diff --git a/tests/test_config.py b/tests/test_config.py index c3e511f..94791b8 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -1,4 +1,5 @@ """Tests for Config module.""" + import json import os import tempfile @@ -100,11 +101,8 @@ threshold: 15 finally: os.unlink(temp_path) - def test_env_override(self): + def test_env_override(self, monkeypatch): """Test environment variable override.""" - os.environ["VIDEO_SUMMARY_MIN_AREA"] = "999" - try: - config = Config(None) - assert config["min_area"] == 999 - finally: - del os.environ["VIDEO_SUMMARY_MIN_AREA"] + monkeypatch.setenv("VIDEO_SUMMARY_MIN_AREA", "999") + config = Config(None) + assert config["min_area"] == 999 diff --git a/tests/test_logger.py b/tests/test_logger.py index c401282..db14847 100644 --- a/tests/test_logger.py +++ b/tests/test_logger.py @@ -1,5 +1,7 @@ """Tests for Logger module.""" + import logging +import os import tempfile from Application.Logger import get_logger, setup_logger @@ -25,13 +27,18 @@ class TestLogger: with tempfile.NamedTemporaryFile(mode="w", suffix=".log", delete=False) as f: log_file = f.name - logger = setup_logger(name="test_logger_3", log_file=log_file) - logger.info("Test message") + try: + logger = setup_logger(name="test_logger_3", log_file=log_file) + logger.info("Test message") - # Verify file was created and has content - with open(log_file, "r") as f: - content = f.read() - assert "Test message" in content + # Verify file was created and has content + with open(log_file, "r") as f: + content = f.read() + assert "Test message" in content + finally: + # Cleanup the log file + if os.path.exists(log_file): + os.unlink(log_file) def test_get_logger(self): """Test getting an existing logger."""