Gemini - #6
Conversation
📝 WalkthroughWalkthroughRelease 1.0.5 adds sun-aware exposure logic and polar-day handling, fast overexposure ramp‑down, per‑camera reference_lux, lores-based brightness measurement, FFMPEG deflicker and keogram generation, expanded timelapse CLI/date handling, daily timelapse orchestration with upload, config and docs updates, and broadened tests. Changes
Sequence DiagramssequenceDiagram
participant Camera as Picamera2
participant AutoTL as AdaptiveTimelapse
participant ImgCap as ImageCapture
participant Overlay as Overlay Engine
participant Lores as Lores Stream
rect rgb(235,245,255)
Note over AutoTL,Lores: Enhanced per-frame capture & exposure loop
end
AutoTL->>AutoTL: Determine mode (Day/Night/Transition)
AutoTL->>AutoTL: Check sun elevation (polar override)
AutoTL->>AutoTL: _check_overexposure() / update fast ramp-down state
AutoTL->>ImgCap: capture_frame(mode, calculated_lux)
rect rgb(235,255,235)
Note over ImgCap,Lores: In-memory brightness measurement (Y plane)
ImgCap->>Camera: Request still + lores stream
Camera->>Lores: Return lores YUV420 buffer
ImgCap->>ImgCap: _compute_brightness_from_lores()
ImgCap->>ImgCap: Cache metrics (mean, percentiles, overexposed%)
end
ImgCap->>Overlay: apply_overlay(image, extra_metadata={calculated_lux})
Overlay->>Overlay: Render overlays, save with config.output.quality
AutoTL->>AutoTL: _smooth_lux() -> _calculate_target_exposure_from_lux()
AutoTL->>AutoTL: Apply sequential ramping (shutter → ISO)
AutoTL->>AutoTL: Log transition progress / seed from metadata if applicable
sequenceDiagram
participant Cron as Scheduler
participant DailyTL as daily_timelapse.py
participant MakeTL as make_timelapse.py
participant Storage as Image Storage
participant FFmpeg as FFMPEG (Deflicker)
participant Keogram as create_keogram.py
participant Server as Web API
Cron->>DailyTL: main() [target_date]
DailyTL->>DailyTL: load_config()
alt create timelapse
DailyTL->>MakeTL: subprocess call (--start/--end defaults 05:00)
MakeTL->>Storage: find images in range
MakeTL->>FFmpeg: run encoder with optional deflicker filter
FFmpeg-->>MakeTL: produce video file
MakeTL->>Keogram: create_keogram_from_images()
Keogram-->>MakeTL: produce keogram
end
alt upload enabled
DailyTL->>Server: POST /api/video/upload (multipart + Bearer)
Server-->>DailyTL: 200 OK
else dry-run
DailyTL->>DailyTL: print planned actions (no I/O)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/overlay.py (1)
103-103: Pipeline failure: Unused variabletest_font.The variable is assigned for its side effect (testing font loading) but never used afterwards. Consider either using it in a validation check or storing the result in a way that makes the intent clearer.
🔎 Proposed fix
try: # Load bold font - test_font = ImageFont.truetype(font_path, 20) + # Test that font loads successfully with a sample size + ImageFont.truetype(font_path, 20) logger.debug(f"Loaded font: {font_path}") return font_path # Return path, will load with proper size later except (OSError, IOError):src/capture_image.py (1)
3-3: Pipeline failure: Unused import 'os'.The
osmodule is imported but never used in this file. Remove it to clean up the imports.🔎 Proposed fix
-import os import json import time from datetime import datetimesrc/auto_timelapse.py (2)
8-10: Remove unused import (pipeline failure: F401).The
osmodule is imported but never used in this file.Suggested fix
-import os import sys import time
1163-1166: Line exceeds 100 characters (pipeline failure: E501).Line 1163 is 134 characters. Consider breaking the complex expression.
Suggested fix
wb_info = ( - f"WB=[{settings.get('ColourGains', ('auto', 'auto'))[0]:.2f}, {settings.get('ColourGains', ('auto', 'auto'))[1]:.2f}]" + f"WB=[{settings.get('ColourGains', ('auto', 'auto'))[0]:.2f}, " + f"{settings.get('ColourGains', ('auto', 'auto'))[1]:.2f}]" if "ColourGains" in settings else "WB=auto" )
🧹 Nitpick comments (23)
docs/TIMELAPSE_VIDEO.md (1)
253-262: Add language specifiers to fenced code blocks.The code blocks showing filename patterns should specify a language (e.g.,
textorbash) for better rendering and accessibility.🔎 Proposed fix
**Same-day timelapse:** -``` +```text {project}_{YYYY-MM-DD}_{HHMM}-{HHMM}.mp4 Example: kringelen_nord_2025-12-25_0700-1500.mp4Multi-day timelapse:
-+text
{project}{YYYY-MM-DD}{HHMM}to{YYYY-MM-DD}_{HHMM}.mp4
Example: kringelen_nord_2025-12-24_0500_to_2025-12-25_0500.mp4TodoKeogram.md (1)
1-40: Consider removing or archiving this TODO document.The keogram implementation described in this document has already been completed (src/create_keogram.py exists in this PR). Consider either:
- Removing this file if it was temporary planning documentation
- Moving it to a
docs/archive/ordocs/planning/folder for historical reference- Converting it to a completion summary or design document
docs/changelog_2025-12-25.md (1)
42-42: Minor style improvement: simplify "reverted back" to "reverted".The phrase "reverted back" is redundant; "reverted" alone conveys the same meaning.
🔎 Proposed fix
-Initially changed to use metadata lux from test shot, but reverted back to calculated lux: +Initially changed to use metadata lux from test shot, but reverted to calculated lux:GeminiImprovements.md (1)
77-84: Add language specifier to code block.The example log output should specify a language (e.g.,
textorlog) for proper syntax highlighting.🔎 Proposed fix
## Example Log Output -``` +```text [Holy Grail] Seeded WB from AWB: [2.54, 1.62] [Holy Grail] Seeded exposure from last capture: 0.0234s [Holy Grail] Seeded gain from last capture: 1.45 [Holy Grail] Transition seeded - AWB locked, smooth interpolation will prevent flash [Transition] Progress: 45% | Lux: 42.5 | Shutter: 234ms | Gain: 1.85 | AWB: Locked</details> </blockquote></details> <details> <summary>src/create_keogram.py (7)</summary><blockquote> `37-72`: **Extract Colors class to eliminate code duplication.** The `Colors` class is duplicated from `src/make_timelapse.py`. Consider extracting it to a shared utility module (e.g., `src/terminal_colors.py` or `src/utils.py`) to maintain DRY principles and ensure consistent terminal output across scripts. <details> <summary>🔎 Example refactor</summary> Create `src/terminal_colors.py`: ```python """ANSI color codes for terminal output.""" class Colors: """ANSI color codes for terminal output.""" HEADER = "\033[95m" BLUE = "\033[94m" CYAN = "\033[96m" GREEN = "\033[92m" YELLOW = "\033[93m" RED = "\033[91m" BOLD = "\033[1m" END = "\033[0m" @staticmethod def header(text: str) -> str: return f"{Colors.BOLD}{Colors.CYAN}{text}{Colors.END}" # ... other methodsThen import in both files:
from src.terminal_colors import Colors
149-158: Improve exception handling specificity.Consider catching specific PIL exceptions (e.g.,
PIL.UnidentifiedImageError,OSError) instead of broadException, and uselogger.exception()instead oflogger.error()to capture the full traceback.🔎 Proposed fix
try: with Image.open(image_paths[0]) as first_img: original_height = first_img.height first_width = first_img.width - except Exception as e: + except (OSError, PIL.UnidentifiedImageError) as e: msg = f"Failed to read first image: {e}" print(Colors.error(f"✗ {msg}")) if logger: - logger.error(msg) + logger.exception(msg) return False
184-217: Improve exception handling in image processing loop.Use specific exceptions and
logger.exception()to capture full error context for debugging.🔎 Proposed fix
try: with Image.open(img_path) as img: # ... processing logic ... - except Exception as e: + except (OSError, PIL.UnidentifiedImageError, ValueError) as e: skipped += 1 if logger: - logger.warning(f"Failed to process {img_path.name}: {e}") + logger.exception(f"Failed to process {img_path.name}") continue
225-254: Improve exception handling when saving keogram.Use specific exceptions and
logger.exception()for better error diagnostics.🔎 Proposed fix
try: # Ensure output directory exists output_path.parent.mkdir(parents=True, exist_ok=True) keogram.save(str(output_path), "JPEG", quality=quality, optimize=True) size_kb = output_path.stat().st_size / 1024 # ... success logging ... return True - except Exception as e: + except (OSError, ValueError) as e: msg = f"Failed to save keogram: {e}" print(Colors.error(f"✗ {msg}")) if logger: - logger.error(msg) + logger.exception(msg) return False
113-254: Consider simplifying the create_keogram function.The function has a cyclomatic complexity of 22, which can make it harder to test and maintain. Consider extracting helper functions for:
- Image dimension validation and cropping calculation
- Individual image processing (load, resize, crop, paste)
- Result reporting and statistics
This would improve readability and testability.
291-450: Consider simplifying the main function.The function has a cyclomatic complexity of 14. Consider extracting helper functions for:
- Output path determination logic (lines 398-425)
- Image discovery and validation
- Argument parsing and validation
This would make the main flow easier to follow and individual pieces easier to test.
367-372: Avoid catching broad Exception during logger setup.Be more specific about which exceptions you expect during logger initialization, or let the exception propagate if logger setup is critical.
🔎 Proposed fix
try: logger = get_logger("create_keogram", args.config) - except Exception: + except (FileNotFoundError, KeyError, ValueError): logger = logging.getLogger("create_keogram") logger.setLevel(logging.INFO)GeminiAPlus.md (1)
1-51: Clear implementation plan with well-defined goals.This planning document effectively outlines the major features to implement: Polar Awareness, Sequential Ramping, EV Safety Clamp, Transition Hysteresis, and Metadata/Analysis enhancements. The execution steps are clear and actionable.
The markdown list indentation could be adjusted to follow standard conventions (2-space indentation for nested lists instead of 4), but this is purely cosmetic for a planning document.
src/analyze_timelapse.py (1)
1012-1016: Unused function parameters in helper function.The
add_zone_shadinghelper function declaresy_minandy_maxparameters but never uses them. These may be remnants from an earlier implementation or intended for future vertical positioning logic.🔎 Proposed fix
- def add_zone_shading(ax, zones, y_min, y_max): + def add_zone_shading(ax, zones): for start, end, mode in zones: color, alpha = mode_colors.get(mode, ("#888888", 0.1)) ax.axvspan(start, end, alpha=alpha, color=color, zorder=0)Then update the calls to this function (lines 1029, 1153-1154, 1320, 1407) to remove the unused arguments.
src/make_timelapse.py (1)
352-710: Consider refactoringmain()to reduce complexity (pipeline failure: C901).The
main()function has a cyclomatic complexity of 41, significantly exceeding typical thresholds. Consider extracting logical segments into helper functions:
parse_and_validate_args()for argument parsing and validationdetermine_time_range()for the date/time logicexecute_video_generation()for the video creation flowexecute_keogram_generation()for the keogram flowThis would improve testability and maintainability. Similarly,
create_video()(complexity 16) could have filter chain building extracted.docs/DAILY_VIDEO.md (2)
106-118: Add language specifier to fenced code block.The code block is missing a language specifier, which helps with syntax highlighting and accessibility.
Suggested fix
-``` +```text usage: daily_timelapse.py [-h] [--date DATE] [-c CONFIG] [--no-upload] [--only-upload] [--dry-run]
246-256: Add language specifier to fenced code block.The API request block should have a language specifier for clarity.
Suggested fix
-``` +```http POST /api/video/upload Authorization: Bearer <api_key> Content-Type: multipart/form-datasrc/auto_timelapse.py (2)
173-197: Fix implicit Optional type hint.Per PEP 484, use explicit
Optional[float]instead of implicit= Nonedefault withfloattype.Suggested fix
+from typing import Dict, Optional, Tuple + - def _is_polar_day(self, lux: float = None) -> bool: + def _is_polar_day(self, lux: Optional[float] = None) -> bool:Note: Similar fixes needed at lines 354, 824, and 1502 for
speed_override,capture_metadata, andcalculated_luxparameters.
1658-1663: Consider usinglogger.exceptionfor unexpected errors.When catching a broad
Exceptionin the test shot failure handler, usinglogger.exceptionwould automatically include the stack trace.Suggested fix
except Exception as e: - logger.error(f"Test shot failed: {e}") + logger.exception(f"Test shot failed: {e}") # Fall back to last mode or day mode mode = self._last_mode or LightMode.DAYsrc/daily_timelapse.py (2)
104-117: Consider using context managers for file handling.The current pattern manually tracks file handles for cleanup. Using
withstatements or a helper would be cleaner and safer.Suggested pattern
def upload_to_server(...) -> bool: if not video_path or not video_path.exists(): logger.error(f"Video file not found: {video_path}") return False try: with open(video_path, "rb") as video_file: files = {"video": video_file} # Conditionally add keogram keogram_file = None if keogram_path and keogram_path.exists(): keogram_file = open(keogram_path, "rb") files["keogram"] = keogram_file try: response = requests.post(url, files=files, data=data, headers=headers, timeout=300) # ... handle response finally: if keogram_file: keogram_file.close() except ...
145-150: Uselogger.exceptionfor better error diagnostics.When logging exceptions,
logger.exceptionautomatically includes the stack trace.Suggested fix
except requests.exceptions.RequestException as e: - logger.error(f"Upload request failed: {e}") + logger.exception(f"Upload request failed: {e}") return False except Exception as e: - logger.error(f"Upload error: {e}") + logger.exception(f"Upload error: {e}") return FalseworkLogs/2025-12-24.md (1)
127-129: Add language specifier to code block.The log format example should have a language specifier.
Suggested fix
-``` +```text [Transition] Progress: 45% | Lux: 42.5 | Shutter: 234ms | Gain: 1.85 | AWB: Locked</details> </blockquote></details> <details> <summary>config/spjutvika.yml (1)</summary><blockquote> `48-52`: **Consider using a slower preset for better quality and compression.** The `ultrafast` preset prioritizes encoding speed, which results in larger files and reduced compression efficiency. Since timelapse video generation is typically not time-critical, consider using `medium` or `slow` preset for better quality at the same file size. <details> <summary>Alternative preset configuration</summary> ```diff codec: name: "libx264" pixel_format: "yuv420p" - preset: "ultrafast" + preset: "medium" threads: 2 crf: 23spjutvika.yml (1)
88-117: Document additional video configuration options.The template is missing several video configuration options that are present in
config/spjutvika.yml:
organize_by_date(boolean) - organizes videos by datedate_format(string) - date format for video organizationcodec.preset(string) - encoding presetcodec.threads(integer) - number of encoding threadsConsider adding these to the template with documentation so users know these options are available.
Additional video configuration options
# Video Output Settings video: # Directory for generated timelapse videos directory: "videos" + # Create subdirectories by date (YEAR/MONTH structure recommended for videos) + organize_by_date: true + + # Date format for subdirectories (if organize_by_date is true) + date_format: "%Y/%m" + # Video filename pattern (supports strftime formatting) # Available placeholders: # - {name} - Project name # - {start_date} - Start date (YYYY-MM-DD) # - {end_date} - End date (YYYY-MM-DD) # You can also use strftime directives like %Y%m%d filename_pattern: "{name}_{start_date}_to_{end_date}.mp4" # Video codec settings codec: # Video codec (libx264 for H.264, libx265 for H.265/HEVC) name: "libx264" # Pixel format (yuv420p for maximum compatibility) pixel_format: "yuv420p" + # Encoding preset (ultrafast, fast, medium, slow, veryslow) + # Slower = better compression and quality, but takes longer + preset: "medium" + + # Number of encoding threads (0 = auto, or specify 1-4) + threads: 0 + # Constant Rate Factor (0-51, lower = better quality, 18-23 recommended) # 18 = visually lossless, 23 = good quality, 28 = acceptable crf: 20
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
graphs/timelapse_analysis_24h.xlsxis excluded by!**/*.xlsx
📒 Files selected for processing (24)
CHANGELOG.mdGeminiAPlus.mdGeminiImprovements.mdImprovements_done.mdREADME.mdTodoKeogram.mdconfig/config.example.ymlconfig/spjutvika.ymldocs/DAILY_VIDEO.mddocs/TIMELAPSE_VIDEO.mddocs/TRANSITION_SMOOTHING.mddocs/changelog_2025-12-25.mdrequirements.txtspjutvika.ymlsrc/analyze_timelapse.pysrc/auto_timelapse.pysrc/capture_image.pysrc/create_keogram.pysrc/daily_timelapse.pysrc/make_timelapse.pysrc/overlay.pytests/test_capture_image.pyworkLogs/2025-12-24.mdworkLogs/2025-12-25.md
🧰 Additional context used
🧬 Code graph analysis (5)
src/daily_timelapse.py (2)
src/logging_config.py (1)
get_logger(145-162)src/capture_image.py (1)
close(557-563)
src/create_keogram.py (2)
src/logging_config.py (1)
get_logger(145-162)src/make_timelapse.py (8)
Colors(34-69)header(48-49)success(52-53)error(56-57)warning(60-61)info(64-65)bold(68-69)print_section(72-76)
tests/test_capture_image.py (1)
src/capture_image.py (1)
capture(402-508)
src/make_timelapse.py (2)
src/create_keogram.py (9)
create_keogram(113-254)create_keogram_from_images(257-288)print_info(81-83)Colors(37-71)bold(70-71)error(58-59)info(66-67)warning(62-63)print_section(74-78)src/logging_config.py (1)
get_logger(145-162)
src/auto_timelapse.py (2)
src/capture_image.py (1)
capture(402-508)tests/test_overlay.py (1)
test_metadata(57-66)
🪛 GitHub Actions: Tests
src/overlay.py
[error] 103-103: F841 local variable 'test_font' is assigned to but never used.
[error] 228-228: C901 'ImageOverlay._prepare_overlay_data' is too complex (14).
[error] 409-409: C901 'ImageOverlay._get_text_lines' is too complex (14).
[error] 536-536: C901 'ImageOverlay.apply_overlay' is too complex (38).
src/daily_timelapse.py
[error] 67-67: F541 f-string is missing placeholders.
[error] 160-160: C901 'main' is too complex (18).
src/create_keogram.py
[error] 20-20: F401 'typing.Tuple' imported but unused.
[error] 113-113: C901 'create_keogram' is too complex (22).
[error] 291-291: C901 'main' is too complex (14).
src/analyze_timelapse.py
[error] 38-45: E402 module level import not at top of file (and related subsequent import order issues detected in the same block).
[error] 54-54: C901 'find_recent_images' is too complex (11).
[error] 211-211: E722 do not use bare 'except'.
[error] 263-263: E501 line too long (114 > 100 characters).
[error] 298-298: C901 'create_graphs' is too complex (35).
[error] 1538-1538: F541 f-string is missing placeholders.
[error] 1543-1543: F541 f-string is missing placeholders.
[error] 1544-1544: E741 ambiguous variable name 'l'.
[error] 1551-1551: F541 f-string is missing placeholders.
[error] 1557-1557: F541 f-string is missing placeholders.
[error] 1563-1563: F541 f-string is missing placeholders.
[error] 1568-1568: F541 f-string is missing placeholders.
[error] 1576-1576: C901 'export_to_excel' is too complex (18).
[error] 1706-1706: E722 do not use bare 'except'.
[error] 1733-1733: E741 ambiguous variable name 'l'.
[error] 1874-1876: E741 ambiguous variable name 'l'.
[error] 1895-1895: E722 do not use bare 'except'.
[error] 1952-1952: F541 f-string is missing placeholders.
[error] 1983-1983: F541 f-string is missing placeholders.
[error] 1985-1985: F541 f-string is missing placeholders.
[error] 1985-1985: F541 f-string is missing placeholders.
[error] 1985-1985: F541 f-string is missing placeholders.
src/capture_image.py
[error] 3-3: F401 'os' imported but unused.
[error] 36-36: F541 f-string is missing placeholders.
[error] 48-48: F541 f-string is missing placeholders.
[error] 183-183: E501 line too long (104 > 100 characters).
[error] 209-209: F811 redefinition of unused 'libcamera' from line 139.
[error] 212-212: E501 line too long (118 > 100 characters).
[error] 235-235: C901 'ImageCapture._prepare_control_map' is too complex (21).
[error] 388-388: E501 line too long (102 > 100 characters).
[error] 402-402: C901 'ImageCapture.capture' is too complex (14).
src/make_timelapse.py
[error] 188-188: C901 'create_video' is too complex (16).
[error] 352-352: C901 'main' is too complex (41).
[error] 391-391: E501 line too long (103 > 100 characters).
src/auto_timelapse.py
[error] 8-8: F401 'os' imported but unused.
[error] 194-194: E501 line too long (108 > 100 characters).
[error] 576-576: E501 line too long (103 > 100 characters).
[error] 701-701: F401 'math' imported but unused.
[error] 981-981: E501 line too long (111 > 100 characters).
[error] 1066-1066: C901 'AdaptiveTimelapse.get_camera_settings' is too complex (11).
[error] 1163-1163: E501 line too long (134 > 100 characters).
[error] 1553-1553: C901 'AdaptiveTimelapse.run' is too complex (28).
🪛 LanguageTool
Improvements_done.md
[grammar] ~10-~10: Use a hyphen to join words.
Context: ...nused config keys - Phase 1.2: Add fixed day WB gains config option - Phase 1.3: ...
(QB_NEW_EN_HYPHEN)
docs/DAILY_VIDEO.md
[grammar] ~14-~14: Ensure spelling is correct
Context: ...cally uploads video and keogram to your webserver - Smart naming: Videos named `{project_n...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
docs/changelog_2025-12-25.md
[style] ~42-~42: Consider using just “reverted”.
Context: ...to use metadata lux from test shot, but reverted back to calculated lux: - Calculated lux is ...
(RETURN_BACK)
🪛 markdownlint-cli2 (0.18.1)
docs/TIMELAPSE_VIDEO.md
253-253: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
259-259: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
GeminiAPlus.md
23-23: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
24-24: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
39-39: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
40-40: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
GeminiImprovements.md
78-78: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/DAILY_VIDEO.md
106-106: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
246-246: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
workLogs/2025-12-24.md
127-127: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
232-232: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.10)
src/daily_timelapse.py
67-67: f-string without any placeholders
Remove extraneous f prefix
(F541)
146-146: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
148-148: Do not catch blind exception: Exception
(BLE001)
149-149: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
156-156: Do not use bare except
(E722)
156-157: try-except-pass detected, consider logging the exception
(S110)
276-276: subprocess call: check for execution of untrusted input
(S603)
280-280: f-string without any placeholders
Remove extraneous f prefix
(F541)
294-294: f-string without any placeholders
Remove extraneous f prefix
(F541)
305-305: f-string without any placeholders
Remove extraneous f prefix
(F541)
src/create_keogram.py
100-100: Avoid specifying long messages outside the exception class
(TRY003)
153-153: Do not catch blind exception: Exception
(BLE001)
157-157: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
213-213: Do not catch blind exception: Exception
(BLE001)
247-247: Consider moving this statement to an else block
(TRY300)
249-249: Do not catch blind exception: Exception
(BLE001)
253-253: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
369-369: Do not catch blind exception: Exception
(BLE001)
384-384: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
416-416: Do not catch blind exception: Exception
(BLE001)
src/analyze_timelapse.py
1012-1012: Unused function argument: y_min
(ARG001)
1012-1012: Unused function argument: y_max
(ARG001)
1388-1388: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
1524-1524: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
src/capture_image.py
364-364: Consider moving this statement to an else block
(TRY300)
366-366: Do not catch blind exception: Exception
(BLE001)
src/make_timelapse.py
462-462: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
477-477: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
src/auto_timelapse.py
150-150: Do not catch blind exception: Exception
(BLE001)
168-168: Consider moving this statement to an else block
(TRY300)
169-169: Do not catch blind exception: Exception
(BLE001)
173-173: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
354-354: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
824-824: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
1502-1502: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
1658-1658: Do not catch blind exception: Exception
(BLE001)
1659-1659: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1723-1723: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (36)
README.md (1)
1-194: Documentation updates look comprehensive!The README effectively communicates the new features (fast overexposure detection, per-camera brightness tuning, deflicker support) and provides clear usage examples with the updated CLI.
workLogs/2025-12-25.md (1)
1-73: Excellent work log documentation!The detailed session notes, including problems addressed, root causes, and lessons learned, provide valuable context for future maintenance and debugging.
requirements.txt (1)
16-17: Well-documented dependency addition!The astral library addition is properly documented with its purpose (sun position calculation for polar regions), and the version constraint allows for compatible updates.
tests/test_capture_image.py (1)
596-621: LGTM! Excellent regression test for a critical bug.The test properly validates the lores stream format requirement (YUV420) that Picamera2 enforces. The clear error message and bug documentation are particularly valuable for future maintainers.
src/overlay.py (1)
778-779: Good improvement: makes output quality configurable.The change correctly reads the quality setting from configuration, allowing per-project quality tuning for overlaid images.
CHANGELOG.md (1)
8-50: LGTM! Comprehensive and well-structured release notes.The changelog clearly documents all major features and fixes in this release. The explanations are concise yet informative, and the formatting follows the Keep a Changelog standard.
docs/TRANSITION_SMOOTHING.md (2)
209-239: Excellent technical documentation for lores stream brightness measurement.The section clearly explains the implementation, benefits, and code structure. The example code snippet effectively illustrates the grayscale conversion using standard luminance coefficients (0.299R + 0.587G + 0.114*B).
482-510: Well-documented changelog entries with clear technical details.The changelog effectively documents the overexposure detection, configurable reference_lux, and lores stream improvements. The explanations provide good context for why these changes were made.
src/analyze_timelapse.py (2)
271-296: Well-structured utility function for mode transition tracking.The
find_transition_zonesfunction clearly identifies continuous time ranges for each mode. The logic is straightforward and handles edge cases (empty inputs, final zone) correctly.
990-1525: Comprehensive Holy Grail transition analysis suite.The new visualization graphs (comparison plot, AWB stability, transition factor, sun elevation) provide excellent diagnostic capabilities for verifying smooth day/night transitions. The mode-aware zone shading and detailed metrics (e.g., AWB jitter calculation) are particularly valuable.
src/capture_image.py (3)
189-190: Critical: Lores stream correctly configured with YUV420 format.The lores stream is properly configured with YUV420 format as required by Picamera2. This matches the test validation in
test_lores_stream_format_must_be_yuvand enables fast in-memory brightness measurement without disk I/O overhead.Also applies to: 201-202
307-369: Excellent implementation of lores brightness analysis.The
_compute_brightness_from_loresmethod correctly extracts the Y (luminance) plane from YUV420 format and computes comprehensive brightness metrics. The approach is efficient (only 320×240 = 76,800 pixels) and provides rich diagnostic data (mean, median, percentiles, under/overexposed percentages).Key strengths:
- Correct YUV420 Y-plane extraction (first 240 rows)
- Proper numpy usage for vectorized operations
- Comprehensive error handling with logging
- Clear documentation of benefits
474-477: Good: Extra metadata merge enables calculated lux propagation.The extra_metadata merge allows upstream components (e.g.,
auto_timelapse.py) to pass calculated lux values and other diagnostics into the capture metadata, overriding camera-derived values when needed. This is essential for accurate overlay display.config/config.example.yml (3)
10-23: Excellent documentation for Location/Polar Awareness configuration.The new Location block is comprehensive and includes clear explanations of each field. The civil twilight threshold documentation effectively explains the "Civil Day Override" behavior at high latitudes.
241-246: Good: Sequential ramping flag with clear explanation.The
sequential_rampingconfiguration is well-documented with a clear explanation of the two-phase ramping process (shutter priority, then gain). This replaces the previous min/max gain approach with a more sophisticated noise-reduction strategy.
323-327: Useful: Test shot frequency control reduces overhead.The new
frequencysetting allows tuning of test shot cadence, which can reduce processing overhead in stable lighting conditions while maintaining responsiveness during transitions.Improvements_done.md (1)
1-269: Documentation looks comprehensive and well-structured.The document provides clear explanations of each implemented phase with before/after code examples, test results, and rationale for skipped phases. This serves as valuable internal documentation.
src/make_timelapse.py (2)
199-216: Deflicker integration looks good.The new
deflickeranddeflicker_sizeparameters with sensible defaults enable FFmpeg's deflicker filter for smoother timelapse transitions. The implementation correctly builds the filter chain and logs the configuration.
665-690: Keogram integration is well-implemented.The keogram generation flow correctly:
- Handles
--no-keogramand--keogram-onlymodes- Generates consistent filenames alongside video files
- Reports success/failure appropriately
- Integrates with the existing logging infrastructure
docs/DAILY_VIDEO.md (1)
1-262: Documentation is comprehensive and well-organized.The documentation clearly explains the daily video workflow, configuration options, troubleshooting steps, and server API requirements. The 05:00-05:00 window convention and keogram integration are well documented.
src/auto_timelapse.py (5)
17-25: Graceful degradation for optional Astral dependency looks good.The try/except pattern for importing Astral and setting
ASTRAL_AVAILABLEallows the module to work without the polar-awareness feature when Astral isn't installed.
581-679: Sequential ramping implementation is well-designed.The two-phase approach (shutter priority first, then gain priority) minimizes sensor noise by keeping ISO low as long as possible. The logarithmic interpolation provides smooth perceived brightness changes. Good defensive checks for edge cases (e.g.,
ev_seed <= 0).
768-822: EV safety clamp ensures seamless auto-to-manual handover.This is a smart approach to prevent the "flash" artifact: if the proposed manual EV differs by more than 5% from the seeded auto EV, it clamps the exposure to match exactly. The 5% threshold is reasonable for imperceptible transitions.
472-521: Overexposure detection with hysteresis is well-implemented.The different trigger (180 brightness, 10% clipped) vs. clear (150 brightness, 5% clipped) thresholds prevent oscillation in the fast ramp-down behavior. The logging at state transitions aids debugging.
1631-1653: Holy Grail seeding integration in run loop is correct.The mode transition detection (
entering_manual_mode) correctly identifies when seeding should occur, and the reset logic when returning to day mode ensures the state machine works correctly across multiple day/night cycles.src/daily_timelapse.py (2)
245-253: Hardcoded fallback path may be fragile.The fallback to
/home/pi/raspberrypi-picamera-timelapse/config.yamlassumes a specific installation location. Consider making this configurable or documenting this assumption.
39-81: File finding functions are well-implemented.The
find_video_fileandfind_keogram_filefunctions use appropriate glob patterns with fallback strategies. Returning the most recent match (via sorting) is a sensible default.workLogs/2025-12-24.md (2)
228-261: Valuable bug fix documentation with regression test.The lores stream format bug fix is well-documented with root cause analysis, the fix, and importantly, a regression test (
test_lores_stream_format_must_be_yuv) to prevent recurrence. This is good engineering practice.
1-227: Comprehensive work log documenting the changes.The work log provides excellent internal documentation of the changes, including code snippets, rationale, and verification steps. The structure (Performance & Config, Holy Grail, Analysis, Bug Fixes) makes it easy to understand the scope of work.
config/spjutvika.yml (5)
5-12: LGTM! Polar awareness configuration is well-designed.The location settings correctly support polar day/night detection at 68.7°N with the civil twilight override forcing day mode when sun elevation exceeds -6°. This addresses the unique challenges of timelapse photography at high latitudes.
69-71: Verify light thresholds are appropriate for this location.The light thresholds (night=5, day=80) are significantly lower than typical values and differ from the template defaults (night=10, day=100). While this may be intentional for the Arctic location's unique lighting conditions, ensure these values have been tested and tuned for Spjutvika's environment.
86-106: LGTM! Advanced Holy Grail transition configuration.The transition mode settings demonstrate sophisticated exposure management with sequential ramping (shutter priority to reduce noise) and brightness feedback for smooth day/night transitions. This aligns well with the gemini branch's Holy Grail transition features.
108-113: Verify thefrequencyparameter is documented.The
test_shot.frequency: 1parameter is not present in the template file (spjutvika.yml). Ensure this parameter is supported by the implementation and document its purpose (e.g., take test shot every N captures).
187-192: Weather API endpoint is accessible and correctly configured.The endpoint is returning HTTP 200 with valid weather data for station
2c4735da-abbe-425e-a2ba-1006e786554c. The station is registered as "Kringelveien 265" and provides the expected measurements (temperature, humidity, CO2, noise, pressure).spjutvika.yml (2)
267-334: LGTM! Comprehensive overlay variable documentation.The extensive documentation of available template variables for overlay content is excellent. It clearly explains camera metadata, weather data, and system monitoring variables with examples, making it easy for users to customize their overlays.
153-170: Removemin_exposure_timefrom the night_mode template — it is unsupported.The template includes
min_exposure_time: 1.0at line 159, but this parameter is never read or used by the codebase. Exposure time clamping usesmax_exposure_timefrom night_mode and minimum values from day_mode settings or hardcoded limits. Remove the unused configuration entry to avoid confusion.Likely an incorrect or invalid review comment.
| # Raspilapse Configuration File (Example/Template) | ||
| # | ||
| # IMPORTANT: This is the default configuration template. | ||
| # Copy this file to config.yml and customize it: | ||
| # cp config/config.example.yml config/config.yml | ||
| # | ||
| # Your config.yml will NOT be tracked by git, so you can safely | ||
| # customize it with your personal settings (API keys, paths, etc.) | ||
|
|
There was a problem hiding this comment.
Missing location configuration section for polar awareness.
This template is missing the location section that defines latitude, longitude, timezone, and civil_twilight_threshold. The location configuration is essential for the polar awareness and sun elevation tracking features mentioned in the PR objectives and is present in config/spjutvika.yml. Users copying this template won't know this feature exists.
Proposed addition of location section
Add this section after line 9, before the Camera Settings:
+# Location Settings (for sun position calculation)
+# Used for Polar Day/Night detection at high latitudes
+# Enables "Civil Twilight Override" - forces Day mode when sun > civil_twilight_threshold
+location:
+ # Geographic coordinates
+ latitude: 0.0 # Example: 68.7 for northern Norway
+ longitude: 0.0 # Example: 15.4 for northern Norway
+
+ # Timezone (e.g., "Europe/Oslo", "America/New_York", "UTC")
+ timezone: "UTC"
+
+ # Civil twilight threshold (degrees)
+ # Day mode is forced when sun elevation > this value
+ # Typical: -6.0 (civil twilight)
+ civil_twilight_threshold: -6.0
+
# Camera SettingsCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In spjutvika.yml around lines 1 to 9, the template lacks a required location
configuration for polar awareness; add a new top-level location section (placed
after line 9 and before the Camera Settings) that defines latitude, longitude,
timezone, and civil_twilight_threshold, matching the format used in
config/spjutvika.yml so users copying the template see and can set these values
(use clear placeholder values and comments for each field).
| # Transition mode settings (dawn/dusk) | ||
| transition_mode: | ||
| # Gradually adjust between day and night settings | ||
| # Based on calculated lux value | ||
| smooth_transition: true | ||
|
|
||
| # Analogue gain range for transition | ||
| analogue_gain_min: 1.0 | ||
| analogue_gain_max: 2.5 |
There was a problem hiding this comment.
Transition mode configuration is incomplete.
The template's transition_mode section is missing numerous advanced settings that are present in config/spjutvika.yml and essential for the Holy Grail transitions feature, including:
sequential_ramping(noise reduction)- Smoothing factors for lux, WB, gain, and exposure
hysteresis_framesbrightness_feedback_enabledand related parameters- Day mode smoothing options
Complete transition_mode configuration
# Transition mode settings (dawn/dusk)
transition_mode:
# Gradually adjust between day and night settings
# Based on calculated lux value
smooth_transition: true
+ # Sequential ramping (shutter first, then gain) - reduces noise
+ sequential_ramping: true
+
+ # Smooth transition settings
+ lux_smoothing_factor: 0.3
+ hysteresis_frames: 3
+ wb_transition_speed: 0.15
+ gain_transition_speed: 0.10
+ exposure_transition_speed: 0.10
+ smooth_wb_in_day_mode: true
+ smooth_exposure_in_day_mode: true
+
+ # Brightness feedback for butter-smooth transitions
+ brightness_feedback_enabled: true
+ target_brightness: 120
+ brightness_tolerance: 40
+ brightness_feedback_strength: 0.2
+
# Analogue gain range for transition
analogue_gain_min: 1.0
analogue_gain_max: 2.5🤖 Prompt for AI Agents
In spjutvika.yml around lines 189 to 197, the transition_mode block is
incomplete: add the missing advanced Holy Grail transition settings
(sequential_ramping, smoothing factors for lux, white_balance, gain, and
exposure, hysteresis_frames, brightness_feedback_enabled and its parameters, and
day_mode smoothing options) to match config/spjutvika.yml; ensure you include
sensible default values and documentation comments for each field, keep naming
consistent with the existing config (e.g., sequential_ramping, lux_smoothing,
wb_smoothing, gain_smoothing, exposure_smoothing, hysteresis_frames,
brightness_feedback_enabled, brightness_feedback_gain,
brightness_feedback_threshold, day_mode: {smooth_transition,
smoothing_factors}), and validate YAML syntax and indentation.
| import argparse | ||
| from datetime import datetime | ||
| from pathlib import Path | ||
| from typing import List, Optional, Tuple |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused import.
The Tuple type from typing is imported but never used in the code.
🔎 Proposed fix
-from typing import List, Optional, Tuple
+from typing import List, Optional📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from typing import List, Optional, Tuple | |
| from typing import List, Optional |
🧰 Tools
🪛 GitHub Actions: Tests
[error] 20-20: F401 'typing.Tuple' imported but unused.
🤖 Prompt for AI Agents
In src/create_keogram.py around line 20, the typing import includes Tuple which
is not used; remove Tuple from the import list so only the actually used types
(e.g., List and Optional) are imported to eliminate the unused-import warning.
| try: | ||
| f.close() | ||
| except: | ||
| pass |
There was a problem hiding this comment.
Avoid bare except with pass.
Using a bare except clause silently ignores all exceptions, including system-exiting ones. At minimum, catch Exception and consider logging.
Suggested fix
finally:
# Close all file handles
for f in file_handles:
try:
f.close()
- except:
- pass
+ except Exception:
+ pass # Best effort cleanupAlternatively, consider using a context manager pattern to handle file cleanup automatically.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| f.close() | |
| except: | |
| pass | |
| try: | |
| f.close() | |
| except Exception: | |
| pass # Best effort cleanup |
🧰 Tools
🪛 Ruff (0.14.10)
156-156: Do not use bare except
(E722)
156-157: try-except-pass detected, consider logging the exception
(S110)
🤖 Prompt for AI Agents
In src/daily_timelapse.py around lines 154 to 157, replace the bare "except:
pass" used when closing the file with a safe exception handling pattern: either
use a context manager ("with open(...) as f:") so the file is closed
automatically, or catch only Exception (e.g., "except Exception as e:") and log
the error instead of silencing it; ensure system-exiting exceptions
(KeyboardInterrupt, SystemExit) are not swallowed and include a brief log
message when close fails.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/make_timelapse.py (1)
607-612: Line exceeds 100 characters (pipeline failure).Line 609 is 136 characters long, exceeding the project's 100-character limit.
🔎 Proposed fix
if args.limit > 0 and len(images) > args.limit: print( - f" {Colors.warning('⚠')} Limiting to first {Colors.bold(str(args.limit))} images {Colors.YELLOW}(testing mode){Colors.END}" + f" {Colors.warning('⚠')} Limiting to first {Colors.bold(str(args.limit))} images " + f"{Colors.YELLOW}(testing mode){Colors.END}" )
♻️ Duplicate comments (5)
src/make_timelapse.py (2)
365-365: Line exceeds 100 characters (pipeline failure).Line 365 is 103 characters long, exceeding the project's 100-character limit.
🔎 Proposed fix
- # Create video from specific dates and times - python3 src/make_timelapse.py --start 07:00 --end 15:00 --start-date 2025-12-24 --end-date 2025-12-25 + # Create video from specific dates and times + python3 src/make_timelapse.py --start 07:00 --end 15:00 \\ + --start-date 2025-12-24 --end-date 2025-12-25
389-396: Line exceeds 100 characters (pipeline failure).Line 391 is 125 characters long, exceeding the project's 100-character limit.
🔎 Proposed fix
parser.add_argument( "--start-date", - help="Start date in YYYY-MM-DD format (e.g., 2025-12-24). Default: yesterday if end time <= start time, else today.", + help="Start date in YYYY-MM-DD format (e.g., 2025-12-24). " + "Default: yesterday if end time <= start time, else today.", )src/daily_timelapse.py (2)
83-157: Replace bareexceptwith specific exception handling (pipeline failure).The bare
except:at line 155 catches all exceptions including system-exiting ones (KeyboardInterrupt, SystemExit), which is flagged as a pipeline failure (E722).🔎 Proposed fix
finally: # Close all file handles for f in file_handles: try: f.close() - except: - pass + except Exception: + pass # Best-effort cleanupAlternatively, consider using context managers (
with open(...)) for automatic file cleanup.
254-283: Remove extraneous f-prefixes from strings without placeholders (pipeline issue).Lines 279, 293, and 304 use f-string prefixes but have no interpolation placeholders, flagged in previous reviews.
🔎 Proposed fix
if result.returncode != 0: logger.error(f"make_timelapse.py failed with code {result.returncode}") - print(f"Error: Timelapse creation failed") + print("Error: Timelapse creation failed") return 1Apply similar fixes at lines 293 and 304.
src/create_keogram.py (1)
20-20: Remove unused import (pipeline failure).The
Tupletype fromtypingis imported but never used in the code, causing a pipeline failure (F401).🔎 Proposed fix
-from typing import List, Optional, Tuple +from typing import List, Optional
🧹 Nitpick comments (3)
docs/TIMELAPSE_VIDEO.md (1)
258-267: Add language specifiers to fenced code blocks.The fenced code blocks at lines 258-260 and 264-267 are missing language identifiers, which improves syntax highlighting and accessibility.
🔎 Proposed fix
**Same-day timelapse:** -``` +```text {project}_{YYYY-MM-DD}_{HHMM}-{HHMM}.mp4 Example: kringelen_nord_2025-12-25_0700-1500.mp4Multi-day timelapse:
-+text
{project}{YYYY-MM-DD}{HHMM}to{YYYY-MM-DD}_{HHMM}.mp4
Example: kringelen_nord_2025-12-24_0500_to_2025-12-25_0500.mp4src/make_timelapse.py (1)
447-541: Consider extracting datetime range calculation to a helper function.The datetime range calculation logic (lines 447-541) is lengthy and adds to the main() complexity (41). Extracting this into a dedicated function would improve readability and testability.
🔎 Suggested refactor
def calculate_datetime_range( args, config: dict, now: datetime ) -> Tuple[datetime, datetime]: """ Calculate start and end datetime from CLI args and config. Returns: Tuple of (start_datetime, end_datetime) Raises: ValueError: If dates/times are invalid or range is invalid """ today = now.date() yesterday = today - timedelta(days=1) # Get defaults from config default_start_time = config.get("video", {}).get("default_start_time", "05:00") default_end_time = config.get("video", {}).get("default_end_time", "05:00") # Parse times... (move existing logic here) # ... # Validate and return if start_datetime >= end_datetime: raise ValueError(f"Start time ({start_datetime}) must be before end time ({end_datetime})") return start_datetime, end_datetime # Then in main(): try: start_datetime, end_datetime = calculate_datetime_range(args, config, now) except ValueError as e: print(Colors.error(f"✗ {e}")) logger.error(str(e)) return 1src/create_keogram.py (1)
36-72: Consider extracting Colors class to a shared utility module.The
Colorsclass is duplicated across multiple files (make_timelapse.py, create_keogram.py). Extracting it to a shared module (e.g.,src/terminal_colors.pyorsrc/cli_utils.py) would eliminate duplication and ensure consistency.🔎 Suggested refactor
Create
src/cli_utils.py:"""Shared CLI utilities for colored terminal output.""" class Colors: """ANSI color codes for terminal output.""" # ... (move entire Colors class here)Then in each file:
try: from src.cli_utils import Colors except ModuleNotFoundError: from cli_utils import Colors
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
CHANGELOG.mddocs/DAILY_VIDEO.mddocs/TIMELAPSE_VIDEO.mdsrc/create_keogram.pysrc/daily_timelapse.pysrc/make_timelapse.pyworkLogs/2025-12-25.md
🧰 Additional context used
🧬 Code graph analysis (3)
src/make_timelapse.py (2)
src/create_keogram.py (9)
create_keogram(113-254)create_keogram_from_images(257-288)print_info(81-83)Colors(37-71)bold(70-71)error(58-59)info(66-67)warning(62-63)print_section(74-78)src/logging_config.py (1)
get_logger(145-162)
src/daily_timelapse.py (4)
src/logging_config.py (1)
get_logger(145-162)src/make_timelapse.py (5)
load_config(90-93)info(64-65)error(56-57)main(352-711)success(52-53)src/create_keogram.py (4)
info(66-67)error(58-59)main(291-450)success(54-55)src/capture_image.py (1)
close(557-563)
src/create_keogram.py (2)
src/logging_config.py (1)
get_logger(145-162)src/make_timelapse.py (7)
Colors(34-69)header(48-49)success(52-53)error(56-57)warning(60-61)info(64-65)bold(68-69)
🪛 GitHub Actions: Tests
src/make_timelapse.py
[error] 188-188: C901 'create_video' is too complex (16).
[error] 352-352: C901 'main' is too complex (41).
[error] 365-365: E501 line too long (103 > 100 characters).
[error] 391-391: E501 line too long (125 > 100 characters).
[error] 609-609: E501 line too long (136 > 100 characters).
[error] 639-639: E501 line too long (148 > 100 characters).
src/daily_timelapse.py
[error] 155-155: E722 do not use bare 'except'.
[error] 159-159: C901 'main' is too complex (18).
src/create_keogram.py
[error] 20-20: F401 'typing.Tuple' imported but unused.
[error] 113-113: C901 'create_keogram' is too complex (22).
[error] 291-291: C901 'main' is too complex (14).
[error] 304-304: E501 line too long (107 > 100 characters).
🪛 LanguageTool
docs/DAILY_VIDEO.md
[grammar] ~14-~14: Ensure spelling is correct
Context: ...cally uploads video and keogram to your webserver - Smart naming: Videos named `{project_n...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
docs/TIMELAPSE_VIDEO.md
258-258: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
264-264: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.10)
src/make_timelapse.py
462-462: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
477-477: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
src/daily_timelapse.py
145-145: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
147-147: Do not catch blind exception: Exception
(BLE001)
148-148: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
155-155: Do not use bare except
(E722)
155-156: try-except-pass detected, consider logging the exception
(S110)
275-275: subprocess call: check for execution of untrusted input
(S603)
279-279: f-string without any placeholders
Remove extraneous f prefix
(F541)
293-293: f-string without any placeholders
Remove extraneous f prefix
(F541)
304-304: f-string without any placeholders
Remove extraneous f prefix
(F541)
src/create_keogram.py
100-100: Avoid specifying long messages outside the exception class
(TRY003)
153-153: Do not catch blind exception: Exception
(BLE001)
157-157: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
213-213: Do not catch blind exception: Exception
(BLE001)
247-247: Consider moving this statement to an else block
(TRY300)
249-249: Do not catch blind exception: Exception
(BLE001)
253-253: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
369-369: Do not catch blind exception: Exception
(BLE001)
384-384: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
416-416: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (9)
workLogs/2025-12-25.md (1)
1-101: Excellent work log documentation.This is a well-structured and detailed work log that effectively documents the session's problems, solutions, code changes, and lessons learned. The two-session breakdown is clear, and the hierarchical organization makes it easy to follow the context and rationale behind each change.
A few minor observations:
Session 2 context (line 77): You might consider noting the wall-clock time for the evening session to make the timeline more explicit (though "Evening" is sufficiently clear in context).
Cross-referencing next steps (line 67–71): The next steps are practical. You may want to add a follow-up work log reference once those items are validated (e.g., reference this date in future work logs if tuning continues).
The documentation clearly communicates intent, trade-offs (e.g., frequent restarts during tuning), and lessons learned—all valuable for future development and debugging.
src/make_timelapse.py (3)
199-216: LGTM! Deflicker integration is well-designed.The deflicker parameters are properly documented and integrated into the video creation flow with sensible defaults.
266-282: LGTM! Filter chain implementation is clean and flexible.The filter chain construction properly handles optional deflicker and scaling filters, building the
-vfargument only when filters are present.
665-691: LGTM! Keogram integration is well-structured.The keogram generation is properly integrated with appropriate success tracking, logging, and conditional execution based on CLI flags.
src/daily_timelapse.py (3)
33-36: LGTM! Config loading is straightforward.The configuration loading function is simple and appropriate for its purpose.
39-80: LGTM! File search logic is comprehensive.The search patterns with recursive glob (
**/) properly handle date-organized directory structures, and the fallback patterns ensure files are found even in different layouts.
259-283: LGTM! Subprocess invocation is well-constructed.Using
sys.executableensures the correct Python interpreter is used, and the command construction with explicit date boundaries is clear and correct.src/create_keogram.py (2)
113-254: LGTM! Keogram generation logic is well-implemented.The core keogram creation function properly handles:
- Resolution changes with appropriate resizing (Lanczos resampling)
- Cropping for overlay removal (7% default)
- Error handling for individual image failures
- Progress reporting
- Comprehensive logging
The 1-pixel center strip extraction and horizontal stitching correctly implements the keogram concept.
397-441: LGTM! CLI integration is robust.The main function includes:
- Smart output path generation from directory structure
- Fallback to current date if directory parsing fails
- Proper crop control via CLI flags
- Comprehensive error handling
| python3 src/create_keogram.py --dir /path/to/images --output keogram_custom.jpg | ||
|
|
||
| # Specify output directory (file will be named keogram_YYYY-MM-DD.jpg) | ||
| python3 src/create_keogram.py --dir /var/www/html/images/2025/12/24/ --output-dir /var/www/html/keograms/ |
There was a problem hiding this comment.
Line exceeds 100 characters (pipeline failure).
Line 304 is 107 characters long, exceeding the project's 100-character limit.
🔎 Proposed fix
- # Specify output directory (file will be named keogram_YYYY-MM-DD.jpg)
- python3 src/create_keogram.py --dir /var/www/html/images/2025/12/24/ --output-dir /var/www/html/keograms/
+ # Specify output directory (file will be named keogram_YYYY-MM-DD.jpg)
+ python3 src/create_keogram.py --dir /var/www/html/images/2025/12/24/ \\
+ --output-dir /var/www/html/keograms/Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: Tests
[error] 304-304: E501 line too long (107 > 100 characters).
🤖 Prompt for AI Agents
In src/create_keogram.py around line 304, the long shell command line (python3
src/create_keogram.py --dir /var/www/html/images/2025/12/24/ --output-dir
/var/www/html/keograms/) exceeds the 100-character limit; shorten it by breaking
the command across two lines (use a shell line-continuation backslash) or
construct the command from smaller parts/variables and concatenate them so no
source line exceeds 100 characters, then run linters to confirm the line-length
violation is fixed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/test_make_timelapse_daily.py (2)
146-148: Strengthen the filename format assertion.The current assertion only checks that "to" exists somewhere in the filename string. Consider using a regex pattern to verify the complete expected format (e.g.,
project_YYYY-MM-DD_HHMM_to_YYYY-MM-DD_HHMM.mp4) to catch malformed filenames that might still contain "to".🔎 Proposed refinement
+import re + # Default 24h range spans two days, so uses _to_ format # e.g., project_2025-12-24_0500_to_2025-12-25_0500.mp4 -assert "_to_" in str(output_file) +# Verify format: name_YYYY-MM-DD_HHMM_to_YYYY-MM-DD_HHMM.mp4 +pattern = r".+_\d{4}-\d{2}-\d{2}_\d{4}_to_\d{4}-\d{2}-\d{2}_\d{4}\.mp4$" +assert re.match(pattern, str(output_file)), f"Unexpected filename format: {output_file}" assert output_file.suffix == ".mp4"
169-173: Verify the specific time range format, not just any dash.The assertion
assert "-" in output_file.stemwill pass as long as there's any dash in the filename stem. For a stem likeproject_2025-12-25_1000-1400, this would pass even if the time range part (1000-1400) is malformed or missing, since the date portion already contains dashes. Consider matching the specific HHMM-HHMM pattern mentioned in the comment.🔎 Proposed refinement
+import re + # Same-day range uses HHMM-HHMM format (no _to_) # e.g., project_2025-12-25_1000-1400.mp4 assert "_to_" not in str(output_file) -# Should have time range with dash separator -assert "-" in output_file.stem # e.g., 1000-1400 +# Should have time range with HHMM-HHMM format +pattern = r".+_\d{4}-\d{4}\.mp4$" +assert re.match(pattern, str(output_file)), f"Expected HHMM-HHMM format in: {output_file}"
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/daily_timelapse.py (2)
145-157: Improve exception handling and logging.
- Use
logging.exceptioninstead oflogging.errorto automatically include the traceback:except requests.exceptions.RequestException as e: - logger.error(f"Upload request failed: {e}") + logger.exception(f"Upload request failed: {e}") return False except Exception as e: - logger.error(f"Upload error: {e}") + logger.exception(f"Upload error: {e}") return False
- The bare
exceptat line 156 was flagged in a previous review and remains unaddressed:for f in file_handles: try: f.close() - except: - pass + except Exception: + pass # Best-effort cleanup
280-280: Remove extraneous f-prefix (static analysis F541).This f-string has no placeholders. Same issue at lines 294 and 305:
Suggested fixes
if result.returncode != 0: logger.error(f"make_timelapse.py failed with code {result.returncode}") - print(f"Error: Timelapse creation failed") + print("Error: Timelapse creation failed") return 1Line 294:
- print(f"Error: Video file not found") + print("Error: Video file not found")Line 305:
- print(f"Would upload:") + print("Would upload:")
🧹 Nitpick comments (1)
src/daily_timelapse.py (1)
245-253: Consider making the legacy config path configurable.The hardcoded path
/home/pi/raspberrypi-picamera-timelapse/config.yamlassumes a specific installation location. For better portability, consider using an environment variable or relative path:old_config_path = os.environ.get( "LEGACY_CONFIG_PATH", "/home/pi/raspberrypi-picamera-timelapse/config.yaml" )This is minor since it's a fallback for legacy migration.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/daily_timelapse.py
🧰 Additional context used
🧬 Code graph analysis (1)
src/daily_timelapse.py (1)
src/logging_config.py (1)
get_logger(145-162)
🪛 GitHub Actions: Tests
src/daily_timelapse.py
[error] 15-15: Library stubs not installed for "yaml" [import-untyped]
[error] 19-19: Library stubs not installed for "requests" [import-untyped]
[error] 36-36: Name "get_logger" already defined (possibly by an import) [no-redef]
[error] 39-39: Returning Any from function declared to return "dict[Any, Any]" [no-any-return]
[error] 39-39: Function "datetime.datetime.date" is not valid as a type [valid-type]
[error] 43-43: datetime.date? has no attribute "strftime" [attr-defined]
[error] 61-61: Function "datetime.datetime.date" is not valid as a type [valid-type]
[error] 63-63: datetime.date? has no attribute "strftime" [attr-defined]
🪛 Ruff (0.14.10)
src/daily_timelapse.py
146-146: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
148-148: Do not catch blind exception: Exception
(BLE001)
149-149: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
156-156: Do not use bare except
(E722)
156-157: try-except-pass detected, consider logging the exception
(S110)
276-276: subprocess call: check for execution of untrusted input
(S603)
280-280: f-string without any placeholders
Remove extraneous f prefix
(F541)
294-294: f-string without any placeholders
Remove extraneous f prefix
(F541)
305-305: f-string without any placeholders
Remove extraneous f prefix
(F541)
🔇 Additional comments (1)
src/daily_timelapse.py (1)
260-270: Good use of sys.executable for subprocess portability.The timelapse command construction correctly uses
sys.executableto ensure the same Python interpreter is used. The 05:00-to-05:00 window spanning two dates properly captures a full 24-hour cycle.
| def find_video_file(video_dir: Path, project_name: str, date: datetime.date) -> Path: | ||
| """Find the generated video file for a given date.""" | ||
| # Look for video with yesterday's date in the filename | ||
| # Format: project_YYYY-MM-DD_0500-0500.mp4 or project_YYYY-MM-DD_0500_to_YYYY-MM-DD_0500.mp4 | ||
| date_str = date.strftime("%Y-%m-%d") | ||
|
|
||
| patterns = [ | ||
| f"**/{project_name}_{date_str}_*.mp4", | ||
| f"**/{project_name}_{date_str}*.mp4", | ||
| f"{project_name}_{date_str}_*.mp4", | ||
| f"{project_name}_{date_str}*.mp4", | ||
| ] | ||
|
|
||
| for pattern in patterns: | ||
| matches = list(video_dir.glob(pattern)) | ||
| if matches: | ||
| # Return most recently modified file (not alphabetically sorted) | ||
| return max(matches, key=lambda p: p.stat().st_mtime) | ||
|
|
||
| return None |
There was a problem hiding this comment.
Fix type annotations to resolve pipeline failures.
The datetime.date type hint is causing mypy errors. Import date directly and add proper return type annotation:
-from datetime import datetime, timedelta
+from datetime import datetime, timedelta, date
+from typing import Optional-def find_video_file(video_dir: Path, project_name: str, date: datetime.date) -> Path:
+def find_video_file(video_dir: Path, project_name: str, target_date: date) -> Optional[Path]:
"""Find the generated video file for a given date."""
# Look for video with yesterday's date in the filename
# Format: project_YYYY-MM-DD_0500-0500.mp4 or project_YYYY-MM-DD_0500_to_YYYY-MM-DD_0500.mp4
- date_str = date.strftime("%Y-%m-%d")
+ date_str = target_date.strftime("%Y-%m-%d")Note: Renaming date parameter to target_date avoids shadowing the imported date type.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: Tests
[error] 39-39: Returning Any from function declared to return "dict[Any, Any]" [no-any-return]
[error] 39-39: Function "datetime.datetime.date" is not valid as a type [valid-type]
[error] 43-43: datetime.date? has no attribute "strftime" [attr-defined]
🤖 Prompt for AI Agents
In src/daily_timelapse.py around lines 39 to 58, the type hints are causing mypy
failures: import date from datetime and typing.Optional, rename the parameter
from date to target_date to avoid shadowing the date type, update the signature
to def find_video_file(video_dir: Path, project_name: str, target_date: date) ->
Optional[Path], adjust the body to use target_date.strftime(...) and ensure
Optional and date are imported at top of the file.
| def find_keogram_file(video_dir: Path, project_name: str, date: datetime.date) -> Path: | ||
| """Find the generated keogram file for a given date.""" | ||
| date_str = date.strftime("%Y-%m-%d") | ||
|
|
||
| patterns = [ | ||
| f"**/keogram_{project_name}_{date_str}*.jpg", | ||
| f"**/keogram*{date_str}*.jpg", | ||
| f"keogram_{project_name}_{date_str}*.jpg", | ||
| f"keogram*{date_str}*.jpg", | ||
| ] | ||
|
|
||
| for pattern in patterns: | ||
| matches = list(video_dir.glob(pattern)) | ||
| if matches: | ||
| # Filter to only include files with the target date | ||
| date_matches = [m for m in matches if date_str in m.name] | ||
| if date_matches: | ||
| # Return most recently modified file (not alphabetically sorted) | ||
| return max(date_matches, key=lambda p: p.stat().st_mtime) | ||
|
|
||
| return None |
There was a problem hiding this comment.
Same type annotation fix needed here.
Apply the same fix as find_video_file:
-def find_keogram_file(video_dir: Path, project_name: str, date: datetime.date) -> Path:
+def find_keogram_file(video_dir: Path, project_name: str, target_date: date) -> Optional[Path]:
"""Find the generated keogram file for a given date."""
- date_str = date.strftime("%Y-%m-%d")
+ date_str = target_date.strftime("%Y-%m-%d")Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: Tests
[error] 61-61: Function "datetime.datetime.date" is not valid as a type [valid-type]
[error] 63-63: datetime.date? has no attribute "strftime" [attr-defined]
🤖 Prompt for AI Agents
In src/daily_timelapse.py around lines 61 to 81, the function find_keogram_file
currently types its return as Path but returns None in the no-match case; change
the signature to return Optional[Path] (import Optional from typing) and update
any callers if needed so the None case is handled — ensure the function
annotation reads def find_keogram_file(...) -> Optional[Path]: and keep the
existing logic including the final return None.
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
src/daily_timelapse.py (5)
39-58: Fix type annotations to resolve pipeline failures.The type annotations have critical issues causing mypy failures:
- Using
datetime.dateas a type hint is invalid; importdatefromdatetime- Parameter name
dateshadows the type, causing attribute errors- Return type is
Pathbut the function returnsNoneon line 58🔎 Proposed fix
Update imports at the top of the file:
-from datetime import datetime, timedelta +from datetime import datetime, timedelta, date +from typing import OptionalFix the function signature and parameter usage:
-def find_video_file(video_dir: Path, project_name: str, date: datetime.date) -> Path: +def find_video_file(video_dir: Path, project_name: str, target_date: date) -> Optional[Path]: """Find the generated video file for a given date.""" # Look for video with yesterday's date in the filename # Format: project_YYYY-MM-DD_0500-0500.mp4 or project_YYYY-MM-DD_0500_to_YYYY-MM-DD_0500.mp4 - date_str = date.strftime("%Y-%m-%d") + date_str = target_date.strftime("%Y-%m-%d")
61-81: Fix type annotations (same issue as find_video_file).Apply the same type annotation fixes:
🔎 Proposed fix
-def find_keogram_file(video_dir: Path, project_name: str, date: datetime.date) -> Path: +def find_keogram_file(video_dir: Path, project_name: str, target_date: date) -> Optional[Path]: """Find the generated keogram file for a given date.""" - date_str = date.strftime("%Y-%m-%d") + date_str = target_date.strftime("%Y-%m-%d")
154-157: Avoid bareexceptin cleanup code.The bare
except: passsilently swallows all exceptions, including system-exiting ones. At minimum, catchExceptionand consider logging failures.🔎 Proposed fix
for f in file_handles: try: f.close() - except: - pass + except Exception as e: + logger.warning(f"Failed to close file handle: {e}")Alternatively, consider using context managers (
with open(...) as f) to handle file cleanup automatically.
288-288: Remove extraneous f-prefix.The f-string has no placeholders.
🔎 Proposed fix
- print(f"Error: Timelapse creation failed") + print("Error: Timelapse creation failed")
302-302: Remove extraneous f-prefixes (multiple instances).Lines 302 and 313 have f-strings without placeholders.
🔎 Proposed fix
logger.error(f"Could not find video file in {video_dir}") - print(f"Error: Video file not found") + print("Error: Video file not found") return 1 logger.info(f"Found video: {video_path}")if args.dry_run: - print(f"Would upload:") + print("Would upload:") print(f" Video: {video_path}")Also applies to: 313-313
🧹 Nitpick comments (1)
src/daily_timelapse.py (1)
145-150: Consider usinglogging.exceptionfor better debugging.Replace
logging.errorwithlogging.exceptionin exception handlers to automatically include stack traces.🔎 Proposed fix
except requests.exceptions.RequestException as e: - logger.error(f"Upload request failed: {e}") + logger.exception("Upload request failed") return False except Exception as e: - logger.error(f"Upload error: {e}") + logger.exception("Upload error") return FalseBased on learnings from Ruff static analysis (TRY400).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/daily_timelapse.py
🧰 Additional context used
🧬 Code graph analysis (1)
src/daily_timelapse.py (1)
src/logging_config.py (1)
get_logger(145-162)
🪛 GitHub Actions: Tests
src/daily_timelapse.py
[error] 15-15: Library stubs not installed for "yaml". (import-untyped)
[error] 19-19: Library stubs not installed for "requests". Hint: "python3 -m pip install types-requests". (import-untyped)
[error] 36-36: Name "get_logger" already defined (possibly by an import). (no-redef)
[error] 39-39: Returning Any from function declared to return "dict[Any, Any]". (no-any-return)
[error] 39-39: Function "datetime.datetime.date" is not valid as a type. Perhaps you need "Callable[...]" or a callback protocol?
[error] 43-43: datetime.date? has no attribute "strftime". (attr-defined)
🪛 Ruff (0.14.10)
src/daily_timelapse.py
146-146: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
148-148: Do not catch blind exception: Exception
(BLE001)
149-149: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
156-156: Do not use bare except
(E722)
156-157: try-except-pass detected, consider logging the exception
(S110)
284-284: subprocess call: check for execution of untrusted input
(S603)
288-288: f-string without any placeholders
Remove extraneous f prefix
(F541)
302-302: f-string without any placeholders
Remove extraneous f prefix
(F541)
313-313: f-string without any placeholders
Remove extraneous f prefix
(F541)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (10)
tests/test_create_keogram.py (2)
293-305:test_create_keogram_varying_resolutionsassertion never checks the “Resized” messageThe final assertion:
assert "Resized" in captured.out or result is Trueis effectively always true because
resultis assertedTrueearlier, so the test doesn’t actually validate the “Resized” log. If you want to assert the log, drop theor result is True:Proposed fix
- captured = capsys.readouterr() - # Should mention resizing - assert "Resized" in captured.out or result is True + captured = capsys.readouterr() + # Should mention resizing + assert "Resized" in captured.out
165-208: Ruff ARG002 on unused fixture parametersTests like
test_find_images_excludes_keograms,test_find_images_excludes_metadata, andtest_main_custom_outputtake fixtures (sample_images,temp_dir) only for their side effects and don’t use the parameter names, which triggers Ruff’s ARG002.To keep the fixture usage while satisfying lint, consider prefixing those parameters with
_or assigning them to_ = sample_imagesinside the test.tests/test_capture_image.py (2)
1192-1238:TestSymlinkLatest.test_symlink_createddoesn’t assert behavior and triggers lint warningsIn
TestSymlinkLatest.test_symlink_created:
image_pathis assigned but never used.symlink_pathis assigned but never asserted.- The test currently only documents “expected behavior” without verifying that a symlink is (or isn’t) created, so it can’t fail.
If the intention is to validate symlink creation, consider asserting on it; if behavior is not yet implemented, mark as
xfailor add a TODO. For example:Proposed assertion-based version
- image_path, _ = capture.capture() - - # Check if symlink was created - symlink_path = os.path.join(test_output_dir, "latest.jpg") - # Note: symlink might not be created if the code path doesn't create it - # This test documents expected behavior + image_path, _ = capture.capture() + + symlink_path = os.path.join(test_output_dir, "latest.jpg") + assert os.path.islink(symlink_path) + assert os.path.realpath(symlink_path) == os.path.realpath(image_path)If you don’t want this assertion yet, at least remove the unused locals or prefix them with
_to appease Ruff.
865-910: Unusedmock_picamera2fixture arguments are intentional but noisy for RuffTests like
test_compute_brightness_metricsandtest_compute_brightness_handles_errortakemock_picamera2only to ensure the Picamera2 module is patched, without using the parameter name in the body, which Ruff flags as ARG002.If you want to keep linters quiet while preserving the fixture hook, rename the parameter to
_mock_picamera2in these tests.tests/test_auto_timelapse.py (1)
1327-1352: Minor: unusedcapsysin CLI testsIn
TestMainFunction.test_main_missing_configandtest_main_help, thecapsysfixture is never used, which Ruff flags as ARG002. Either drop the parameter or add assertions on output viacapsys.readouterr()if you want to validate messages as well as exit codes.tests/test_daily_timelapse.py (2)
475-541:test_main_default_date_yesterdaydoesn’t assert behaviorIn
test_main_default_date_yesterday, you patchsubprocess.runbut never assert how it’s used; the comment says “Should not have called subprocess since --only-upload was used,” yet there is noassertonmock_run.Consider asserting the expected behavior, e.g.:
Proposed improvement
- with patch("daily_timelapse.subprocess.run") as mock_run: - mock_run.return_value = MagicMock(returncode=0) - main() - - # Should not have called subprocess since --only-upload was used + with patch("daily_timelapse.subprocess.run") as mock_run: + mock_run.return_value = MagicMock(returncode=0) + result = main() + + # Should not have called subprocess since --only-upload was used + assert result == 0 + mock_run.assert_not_called()This also resolves Ruff’s F841 “assigned but never used” warning for
resultin similar tests.
79-113: Minor: unusedfilesfrom fixturesIn fixtures like
sample_video_filesandsample_keogram_files, and in some tests where you unpack(video_dir, files), thefileselement isn’t used, which Ruff flags (RUF059). If you only needvideo_dir, unpack as(video_dir, _)or don’t unpackfilesat all.tests/test_make_timelapse.py (2)
160-170: Minor comment/expectation mismatch intest_find_images_single_dayThe comment says “Should find 4 images” but the assertion expects 5:
# Should find 4 images (20:00, 20:30, 21:00, 22:00, 23:00) assert len(images) == 5Either adjust the comment to 5 or drop the list to avoid confusion for future readers.
743-855: Unusedcapsysarguments in main CLI testsIn
TestMainCLI(test_main_help,test_main_missing_config,test_main_invalid_start_time,test_main_invalid_end_time,test_main_invalid_date_format),capsysis passed but never used. You can drop the parameter or add assertions on CLI output viacapsys.readouterr()if that’s useful.tests/test_analyze_timelapse.py (1)
579-629: Minor: unusedcapsysin some CLI testsIn
TestMainCLI.test_main_helpandtest_main_nonexistent_config,capsysis injected but not used. If you don’t need to assert on stdout/stderr, you can remove the parameter; otherwise, add simple checks viacapsys.readouterr()to validate the messages.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
graphs/timelapse_analysis_24h.xlsxis excluded by!**/*.xlsx
📒 Files selected for processing (6)
tests/test_analyze_timelapse.pytests/test_auto_timelapse.pytests/test_capture_image.pytests/test_create_keogram.pytests/test_daily_timelapse.pytests/test_make_timelapse.py
🧰 Additional context used
🧬 Code graph analysis (5)
tests/test_daily_timelapse.py (1)
src/daily_timelapse.py (3)
find_video_file(39-58)find_keogram_file(61-81)upload_to_server(84-157)
tests/test_analyze_timelapse.py (1)
src/analyze_timelapse.py (9)
calculate_image_brightness(113-128)extract_exif_data(131-145)find_transition_zones(271-295)create_graphs(298-1526)main(1905-1990)load_config(48-51)analyze_images(158-268)export_to_excel(1576-1902)find_recent_images(54-110)
tests/test_auto_timelapse.py (1)
src/auto_timelapse.py (10)
AdaptiveTimelapse(46-1766)_is_polar_day(173-197)_get_sun_elevation(154-171)_check_overexposure(472-521)_seed_from_metadata(824-878)LightMode(38-43)_enrich_metadata_with_diagnostics(1373-1463)_create_latest_symlink(1465-1499)_calculate_target_exposure_from_lux(681-743)main(1769-1803)
tests/test_make_timelapse.py (1)
src/make_timelapse.py (15)
parse_time(96-112)find_images_in_range(115-185)load_config(90-93)create_video(188-349)Colors(34-69)print_section(72-76)print_subsection(79-82)print_info(85-87)main(352-711)header(48-49)success(52-53)error(56-57)warning(60-61)info(64-65)bold(68-69)
tests/test_capture_image.py (1)
src/capture_image.py (7)
capture(402-508)ImageCapture(105-572)initialize_camera(127-233)_compute_brightness_from_lores(307-368)_save_metadata_from_dict(510-542)close(557-563)main(594-627)
🪛 GitHub Actions: Tests
tests/test_daily_timelapse.py
[error] 1-1: Pytest collection failed due to ImportError: No module named 'requests'.
🪛 Ruff (0.14.10)
tests/test_daily_timelapse.py
144-144: Unpacked variable files is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
164-164: Unpacked variable files is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
197-197: Unpacked variable files is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
211-211: Unpacked variable files is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
249-249: Unpacked variable files is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
542-542: Unused method argument: temp_dir
(ARG002)
565-565: Unused method argument: temp_dir
(ARG002)
584-584: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
589-589: Unused method argument: temp_dir
(ARG002)
612-612: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
618-618: Unused method argument: temp_dir
(ARG002)
635-635: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
640-640: Unused method argument: temp_dir
(ARG002)
706-706: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
711-711: Unused method argument: temp_dir
(ARG002)
735-735: Unused method argument: temp_dir
(ARG002)
772-772: Unused method argument: capsys
(ARG002)
822-822: Unused method argument: temp_dir
(ARG002)
852-852: Unused method argument: temp_dir
(ARG002)
tests/test_create_keogram.py
165-165: Unused method argument: sample_images
(ARG002)
176-176: Unused method argument: sample_images
(ARG002)
405-405: Unused method argument: temp_dir
(ARG002)
tests/test_analyze_timelapse.py
579-579: Unused method argument: capsys
(ARG002)
588-588: Unused method argument: capsys
(ARG002)
tests/test_auto_timelapse.py
1294-1294: Redefinition of unused TestExposureCalculation from line 752
(F811)
1329-1329: Unused method argument: capsys
(ARG002)
1342-1342: Unused method argument: capsys
(ARG002)
tests/test_make_timelapse.py
743-743: Unused method argument: capsys
(ARG002)
765-765: Unused method argument: capsys
(ARG002)
796-796: Unused method argument: capsys
(ARG002)
826-826: Unused method argument: capsys
(ARG002)
tests/test_capture_image.py
865-865: Unused method argument: mock_picamera2
(ARG002)
894-894: Unused method argument: mock_picamera2
(ARG002)
943-943: Unused method argument: mock_picamera2
(ARG002)
976-976: Unused method argument: mock_picamera2
(ARG002)
1032-1032: Unused method argument: mock_picamera2
(ARG002)
1059-1059: Redefinition of unused TestControlMapping from line 476
(F811)
1091-1091: Unused method argument: mock_picamera2
(ARG002)
1112-1112: Unused method argument: mock_picamera2
(ARG002)
1129-1129: Unused method argument: mock_picamera2
(ARG002)
1177-1177: Unused method argument: mock_picamera2
(ARG002)
1192-1192: Unused method argument: mock_picamera2
(ARG002)
1231-1231: Unpacked variable image_path is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
1234-1234: Local variable symlink_path is assigned to but never used
Remove assignment to unused variable symlink_path
(F841)
🔇 Additional comments (6)
tests/test_create_keogram.py (1)
150-209: Solid coverage forfind_imagesedge casesThe
TestFindImagessuite covers sorting, exclusion of keograms/metadata, custom patterns, non-existent/empty directories. This looks thorough and well-aligned with the intended behavior.tests/test_capture_image.py (1)
596-621: Nice targeted regression test for lores YUV formatThe
test_lores_stream_format_must_be_yuvtest cleanly locks in the YUV420 requirement on the lores stream and would catch any accidental regressions back to RGB888. The use of the existingmock_picamera2fixture and inspection ofcreate_still_configurationkwargs looks good.tests/test_auto_timelapse.py (1)
1015-1102: Good coverage of overexposure fast ramp‑down behaviorThe
TestOverexposureDetectiontests mirror the implementation thresholds (mean > 180 or clipped > 10% to trigger, <150 and <5% to clear, and state retention when metrics are empty/None). This is a nice, focused regression guard around_check_overexposure.tests/test_make_timelapse.py (1)
443-519: Colors and print helper tests align with public APIThe
TestColorsandTestPrintFunctionssuites line up with the documented behavior ofColors,print_section,print_subsection, andprint_infoinmake_timelapse.py. These should give good confidence that future styling changes won’t silently break CLI output.tests/test_analyze_timelapse.py (2)
385-465: Good coverage of brightness and EXIF helper functions
TestCalculateImageBrightnessandTestExtractExifDataexercise the happy paths (white/black/gray images, no-EXIF images) and error paths (invalid/nonexistent files), matching the try/except behavior incalculate_image_brightnessandextract_exif_data. This should prevent regressions in these low-level utilities.
520-574:create_graphsempty-data behavior is well specified
test_create_graphs_empty_dataasserts that callingcreate_graphswith an emptytimestampslist prints “No data to plot” and exits gracefully, aligning with the guard clause increate_graphs. This is a nice protection against accidental crashes when no data is available.
| class TestExposureCalculation: | ||
| """Test exposure calculation from lux values.""" | ||
|
|
||
| def test_calculate_target_exposure_from_lux_night(self, test_config_file): | ||
| """Test exposure calculation for night conditions.""" | ||
| timelapse = AdaptiveTimelapse(test_config_file) | ||
|
|
||
| # Very low lux should give max night exposure | ||
| exposure = timelapse._calculate_target_exposure_from_lux(0.1) | ||
|
|
||
| assert exposure > 10.0 # Should be long exposure | ||
|
|
||
| def test_calculate_target_exposure_from_lux_day(self, test_config_file): | ||
| """Test exposure calculation for day conditions.""" | ||
| timelapse = AdaptiveTimelapse(test_config_file) | ||
|
|
||
| # High lux should give short exposure | ||
| exposure = timelapse._calculate_target_exposure_from_lux(10000.0) | ||
|
|
||
| assert exposure < 0.1 # Should be short exposure | ||
|
|
||
| def test_calculate_target_exposure_from_lux_transition(self, test_config_file): | ||
| """Test exposure calculation for transition conditions.""" | ||
| timelapse = AdaptiveTimelapse(test_config_file) | ||
|
|
||
| # Transition lux should give intermediate exposure | ||
| exposure = timelapse._calculate_target_exposure_from_lux(50.0) | ||
|
|
||
| # Should be between day and night extremes | ||
| assert 0.01 < exposure < 20.0 | ||
|
|
||
|
|
There was a problem hiding this comment.
Duplicate TestExposureCalculation class hides earlier tests
TestExposureCalculation is defined once around line 752 and then redefined here. The later class overwrites the former at module level, so only one set of tests is discovered by pytest.
Rename one of the classes (e.g. TestExposureCalculationRanges) so both groups run:
Proposed rename
-class TestExposureCalculation:
+class TestExposureCalculationRanges:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class TestExposureCalculation: | |
| """Test exposure calculation from lux values.""" | |
| def test_calculate_target_exposure_from_lux_night(self, test_config_file): | |
| """Test exposure calculation for night conditions.""" | |
| timelapse = AdaptiveTimelapse(test_config_file) | |
| # Very low lux should give max night exposure | |
| exposure = timelapse._calculate_target_exposure_from_lux(0.1) | |
| assert exposure > 10.0 # Should be long exposure | |
| def test_calculate_target_exposure_from_lux_day(self, test_config_file): | |
| """Test exposure calculation for day conditions.""" | |
| timelapse = AdaptiveTimelapse(test_config_file) | |
| # High lux should give short exposure | |
| exposure = timelapse._calculate_target_exposure_from_lux(10000.0) | |
| assert exposure < 0.1 # Should be short exposure | |
| def test_calculate_target_exposure_from_lux_transition(self, test_config_file): | |
| """Test exposure calculation for transition conditions.""" | |
| timelapse = AdaptiveTimelapse(test_config_file) | |
| # Transition lux should give intermediate exposure | |
| exposure = timelapse._calculate_target_exposure_from_lux(50.0) | |
| # Should be between day and night extremes | |
| assert 0.01 < exposure < 20.0 | |
| class TestExposureCalculationRanges: | |
| """Test exposure calculation from lux values.""" | |
| def test_calculate_target_exposure_from_lux_night(self, test_config_file): | |
| """Test exposure calculation for night conditions.""" | |
| timelapse = AdaptiveTimelapse(test_config_file) | |
| # Very low lux should give max night exposure | |
| exposure = timelapse._calculate_target_exposure_from_lux(0.1) | |
| assert exposure > 10.0 # Should be long exposure | |
| def test_calculate_target_exposure_from_lux_day(self, test_config_file): | |
| """Test exposure calculation for day conditions.""" | |
| timelapse = AdaptiveTimelapse(test_config_file) | |
| # High lux should give short exposure | |
| exposure = timelapse._calculate_target_exposure_from_lux(10000.0) | |
| assert exposure < 0.1 # Should be short exposure | |
| def test_calculate_target_exposure_from_lux_transition(self, test_config_file): | |
| """Test exposure calculation for transition conditions.""" | |
| timelapse = AdaptiveTimelapse(test_config_file) | |
| # Transition lux should give intermediate exposure | |
| exposure = timelapse._calculate_target_exposure_from_lux(50.0) | |
| # Should be between day and night extremes | |
| assert 0.01 < exposure < 20.0 |
🧰 Tools
🪛 Ruff (0.14.10)
1294-1294: Redefinition of unused TestExposureCalculation from line 752
(F811)
🤖 Prompt for AI Agents
In tests/test_auto_timelapse.py around lines 1294 to 1325, a second
TestExposureCalculation class redefines the earlier one (first defined ~line
752), causing the first set of tests to be overwritten; rename this class (for
example to TestExposureCalculationRanges) so both classes coexist at module
level, update any references if used elsewhere, and run pytest to verify both
groups are discovered.
| class TestControlMapping: | ||
| """Test control key mapping between snake_case and PascalCase.""" | ||
|
|
||
| @pytest.fixture | ||
| def test_config(self, test_output_dir): | ||
| """Create a test config file for control mapping tests.""" | ||
| config_data = { | ||
| "camera": { | ||
| "resolution": {"width": 640, "height": 480}, | ||
| "transforms": {"horizontal_flip": False, "vertical_flip": False}, | ||
| "controls": {}, | ||
| }, | ||
| "output": { | ||
| "directory": test_output_dir, | ||
| "filename_pattern": "test.jpg", | ||
| "project_name": "test", | ||
| "quality": 85, | ||
| "organize_by_date": False, | ||
| }, | ||
| "system": { | ||
| "create_directories": True, | ||
| "save_metadata": True, | ||
| "metadata_filename": "test_metadata.json", | ||
| }, | ||
| "overlay": {"enabled": False}, | ||
| } | ||
|
|
||
| config_path = os.path.join(test_output_dir, "test_config.yml") | ||
| with open(config_path, "w") as f: | ||
| yaml.dump(config_data, f) | ||
| return config_path | ||
|
|
||
| def test_prepare_control_map_snake_case(self, mock_picamera2, test_config): | ||
| """Test mapping snake_case keys to PascalCase.""" | ||
| config = CameraConfig(test_config) | ||
| capture = ImageCapture(config) | ||
|
|
||
| controls = { | ||
| "exposure_time": 10000, | ||
| "analogue_gain": 2.0, | ||
| "awb_enable": True, | ||
| "ae_enable": False, | ||
| "colour_gains": [1.5, 1.3], | ||
| } | ||
|
|
||
| result = capture._prepare_control_map(controls) | ||
|
|
||
| assert result["ExposureTime"] == 10000 | ||
| assert result["AnalogueGain"] == 2.0 | ||
| assert result["AwbEnable"] == 1 | ||
| assert result["AeEnable"] == 0 | ||
| assert result["ColourGains"] == (1.5, 1.3) | ||
|
|
||
| def test_prepare_control_map_pascal_case(self, mock_picamera2, test_config): | ||
| """Test mapping preserves PascalCase keys.""" | ||
| config = CameraConfig(test_config) | ||
| capture = ImageCapture(config) | ||
|
|
||
| controls = { | ||
| "ExposureTime": 20000, | ||
| "AnalogueGain": 4.0, | ||
| "AfMode": 1, | ||
| } | ||
|
|
||
| result = capture._prepare_control_map(controls) | ||
|
|
||
| assert result["ExposureTime"] == 20000 | ||
| assert result["AnalogueGain"] == 4.0 | ||
| assert result["AfMode"] == 1 | ||
|
|
||
| def test_prepare_control_map_mixed(self, mock_picamera2, test_config): | ||
| """Test mapping handles mixed case keys.""" | ||
| config = CameraConfig(test_config) | ||
| capture = ImageCapture(config) | ||
|
|
||
| controls = { | ||
| "exposure_time": 10000, # snake_case | ||
| "AnalogueGain": 3.0, # PascalCase | ||
| } | ||
|
|
||
| result = capture._prepare_control_map(controls) | ||
|
|
||
| assert result["ExposureTime"] == 10000 | ||
| assert result["AnalogueGain"] == 3.0 | ||
|
|
There was a problem hiding this comment.
Duplicate TestControlMapping class shadows earlier tests
There are two TestControlMapping classes in this module (one earlier around line 476, and this new one). The second definition overwrites the first at module level, so pytest will only see the tests in the last class; earlier tests with the same class name won’t run at all.
Rename one of the classes (e.g. TestControlMappingWithConfigFixture) to keep both sets of tests active:
Proposed rename
-class TestControlMapping:
+class TestControlMappingWithConfigFixture:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class TestControlMapping: | |
| """Test control key mapping between snake_case and PascalCase.""" | |
| @pytest.fixture | |
| def test_config(self, test_output_dir): | |
| """Create a test config file for control mapping tests.""" | |
| config_data = { | |
| "camera": { | |
| "resolution": {"width": 640, "height": 480}, | |
| "transforms": {"horizontal_flip": False, "vertical_flip": False}, | |
| "controls": {}, | |
| }, | |
| "output": { | |
| "directory": test_output_dir, | |
| "filename_pattern": "test.jpg", | |
| "project_name": "test", | |
| "quality": 85, | |
| "organize_by_date": False, | |
| }, | |
| "system": { | |
| "create_directories": True, | |
| "save_metadata": True, | |
| "metadata_filename": "test_metadata.json", | |
| }, | |
| "overlay": {"enabled": False}, | |
| } | |
| config_path = os.path.join(test_output_dir, "test_config.yml") | |
| with open(config_path, "w") as f: | |
| yaml.dump(config_data, f) | |
| return config_path | |
| def test_prepare_control_map_snake_case(self, mock_picamera2, test_config): | |
| """Test mapping snake_case keys to PascalCase.""" | |
| config = CameraConfig(test_config) | |
| capture = ImageCapture(config) | |
| controls = { | |
| "exposure_time": 10000, | |
| "analogue_gain": 2.0, | |
| "awb_enable": True, | |
| "ae_enable": False, | |
| "colour_gains": [1.5, 1.3], | |
| } | |
| result = capture._prepare_control_map(controls) | |
| assert result["ExposureTime"] == 10000 | |
| assert result["AnalogueGain"] == 2.0 | |
| assert result["AwbEnable"] == 1 | |
| assert result["AeEnable"] == 0 | |
| assert result["ColourGains"] == (1.5, 1.3) | |
| def test_prepare_control_map_pascal_case(self, mock_picamera2, test_config): | |
| """Test mapping preserves PascalCase keys.""" | |
| config = CameraConfig(test_config) | |
| capture = ImageCapture(config) | |
| controls = { | |
| "ExposureTime": 20000, | |
| "AnalogueGain": 4.0, | |
| "AfMode": 1, | |
| } | |
| result = capture._prepare_control_map(controls) | |
| assert result["ExposureTime"] == 20000 | |
| assert result["AnalogueGain"] == 4.0 | |
| assert result["AfMode"] == 1 | |
| def test_prepare_control_map_mixed(self, mock_picamera2, test_config): | |
| """Test mapping handles mixed case keys.""" | |
| config = CameraConfig(test_config) | |
| capture = ImageCapture(config) | |
| controls = { | |
| "exposure_time": 10000, # snake_case | |
| "AnalogueGain": 3.0, # PascalCase | |
| } | |
| result = capture._prepare_control_map(controls) | |
| assert result["ExposureTime"] == 10000 | |
| assert result["AnalogueGain"] == 3.0 | |
| class TestControlMappingWithConfigFixture: | |
| """Test control key mapping between snake_case and PascalCase.""" | |
| @pytest.fixture | |
| def test_config(self, test_output_dir): | |
| """Create a test config file for control mapping tests.""" | |
| config_data = { | |
| "camera": { | |
| "resolution": {"width": 640, "height": 480}, | |
| "transforms": {"horizontal_flip": False, "vertical_flip": False}, | |
| "controls": {}, | |
| }, | |
| "output": { | |
| "directory": test_output_dir, | |
| "filename_pattern": "test.jpg", | |
| "project_name": "test", | |
| "quality": 85, | |
| "organize_by_date": False, | |
| }, | |
| "system": { | |
| "create_directories": True, | |
| "save_metadata": True, | |
| "metadata_filename": "test_metadata.json", | |
| }, | |
| "overlay": {"enabled": False}, | |
| } | |
| config_path = os.path.join(test_output_dir, "test_config.yml") | |
| with open(config_path, "w") as f: | |
| yaml.dump(config_data, f) | |
| return config_path | |
| def test_prepare_control_map_snake_case(self, mock_picamera2, test_config): | |
| """Test mapping snake_case keys to PascalCase.""" | |
| config = CameraConfig(test_config) | |
| capture = ImageCapture(config) | |
| controls = { | |
| "exposure_time": 10000, | |
| "analogue_gain": 2.0, | |
| "awb_enable": True, | |
| "ae_enable": False, | |
| "colour_gains": [1.5, 1.3], | |
| } | |
| result = capture._prepare_control_map(controls) | |
| assert result["ExposureTime"] == 10000 | |
| assert result["AnalogueGain"] == 2.0 | |
| assert result["AwbEnable"] == 1 | |
| assert result["AeEnable"] == 0 | |
| assert result["ColourGains"] == (1.5, 1.3) | |
| def test_prepare_control_map_pascal_case(self, mock_picamera2, test_config): | |
| """Test mapping preserves PascalCase keys.""" | |
| config = CameraConfig(test_config) | |
| capture = ImageCapture(config) | |
| controls = { | |
| "ExposureTime": 20000, | |
| "AnalogueGain": 4.0, | |
| "AfMode": 1, | |
| } | |
| result = capture._prepare_control_map(controls) | |
| assert result["ExposureTime"] == 20000 | |
| assert result["AnalogueGain"] == 4.0 | |
| assert result["AfMode"] == 1 | |
| def test_prepare_control_map_mixed(self, mock_picamera2, test_config): | |
| """Test mapping handles mixed case keys.""" | |
| config = CameraConfig(test_config) | |
| capture = ImageCapture(config) | |
| controls = { | |
| "exposure_time": 10000, # snake_case | |
| "AnalogueGain": 3.0, # PascalCase | |
| } | |
| result = capture._prepare_control_map(controls) | |
| assert result["ExposureTime"] == 10000 | |
| assert result["AnalogueGain"] == 3.0 |
🧰 Tools
🪛 Ruff (0.14.10)
1059-1059: Redefinition of unused TestControlMapping from line 476
(F811)
1091-1091: Unused method argument: mock_picamera2
(ARG002)
1112-1112: Unused method argument: mock_picamera2
(ARG002)
1129-1129: Unused method argument: mock_picamera2
(ARG002)
🤖 Prompt for AI Agents
In tests/test_capture_image.py around lines 1059-1143, this TestControlMapping
class duplicates an earlier TestControlMapping (around line ~476) which causes
the latter to overwrite the former; rename this class to a unique name (e.g.
TestControlMappingWithConfigFixture) so both sets of tests are discovered, and
update any local references to the class name if present (no other logic changes
required).
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/make_timelapse.py (2)
392-399: Line exceeds 100 characters (previously flagged).This line length issue was already flagged in a previous review. Line 394 is 103 characters long, exceeding the project's 100-character limit.
🔎 Proposed fix (from previous review)
parser.add_argument( "--start-date", - help="Start date in YYYY-MM-DD format (e.g., 2025-12-24). Default: yesterday if end time <= start time, else today.", + help="Start date in YYYY-MM-DD format (e.g., 2025-12-24). " + "Default: yesterday if end time <= start time, else today.", )
662-674: Line exceeds 100 characters (previously flagged).This line length issue was already flagged in a previous review. Line 670 is 148 characters long, exceeding the project's 100-character limit.
🔎 Proposed fix (from previous review)
# If same date, use shorter format if start_datetime.date() == end_datetime.date(): # Same day: projectname_YYYY-MM-DD_HHMM-HHMM.mp4 - filename = f"{project_name}_{start_datetime.strftime('%Y-%m-%d')}_{start_datetime.strftime('%H%M')}-{end_datetime.strftime('%H%M')}.mp4" + filename = ( + f"{project_name}_{start_datetime.strftime('%Y-%m-%d')}_" + f"{start_datetime.strftime('%H%M')}-{end_datetime.strftime('%H%M')}.mp4" + )
🧹 Nitpick comments (1)
src/make_timelapse.py (1)
500-546: Remove unused variableuse_current_time.The variable
use_current_timeis set toFalseat line 500 but is never changed toTrue. The conditional at lines 541-543 that checks this variable is therefore dead code and will never execute.🔎 Proposed fix
- use_current_time = False - # Parse dates if args.start_date: try: @@ ... @@ # Build datetime objects start_datetime = datetime.combine(start_date, datetime.min.time()).replace( hour=start_hour, minute=start_min, second=0, microsecond=0 ) - if use_current_time: - end_datetime = now - else: - end_datetime = datetime.combine(end_date, datetime.min.time()).replace( - hour=end_hour, minute=end_min, second=0, microsecond=0 - ) + end_datetime = datetime.combine(end_date, datetime.min.time()).replace( + hour=end_hour, minute=end_min, second=0, microsecond=0 + )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/make_timelapse.py
🧰 Additional context used
🧬 Code graph analysis (1)
src/make_timelapse.py (2)
src/create_keogram.py (8)
create_keogram(113-254)create_keogram_from_images(257-288)print_info(81-83)Colors(37-71)bold(70-71)error(58-59)info(66-67)warning(62-63)src/logging_config.py (1)
get_logger(145-162)
🪛 Ruff (0.14.10)
src/make_timelapse.py
477-477: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
492-492: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🔇 Additional comments (6)
src/make_timelapse.py (6)
27-30: LGTM!The keogram import with fallback pattern is consistent with the existing
get_loggerimport pattern and ensures the module works in different execution contexts.
266-282: LGTM!The filter chain construction is clean and maintainable. The use of
deflicker=mode=pm(Predictive Mean) is appropriate for timelapse videos, and the conditional application of filters prevents unnecessary-vfflags when no filters are needed.
358-381: LGTM!The CLI examples are comprehensive and clearly demonstrate the new date/time handling features, hardware encoding, and 1080p options.
477-477: Static analysis suggestion can be safely ignored.The static analysis tool suggests using
logging.exceptioninstead oflogging.error, but this is a false positive. This error handling is for expected validation failures (invalid time format), not unexpected exceptions. Usinglogger.errorwithout a traceback is more appropriate here, as it provides a cleaner user experience for validation errors.Based on static analysis hints, but determined to be a false positive.
587-614: LGTM!The configuration loading and CLI override logic is clean and well-structured. The hardware encoder and 1080p flags are properly applied, and the display string accurately reflects the selected settings.
677-742: LGTM! Well-structured conditional execution.The keogram integration is well-designed with clear conditional paths for:
- Video-only mode (default)
- Keogram-only mode (
--keogram-only)- Combined mode (both video and keogram)
- No keogram mode (
--no-keogram)The final status reporting correctly handles all outcome combinations and provides clear feedback to users.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
tests/test_make_timelapse.py (2)
21-21: Remove unusedMagicMockimport.
MagicMockis imported but never used in this file; onlyMockandpatchare utilized.Suggested fix
-from unittest.mock import Mock, patch, MagicMock +from unittest.mock import Mock, patch
761-761: Remove unusedcapsysparameters or verify output content.Several test methods receive
capsysbut don't use it to verify output:
test_main_help(line 761)test_main_invalid_start_time(line 783)test_main_invalid_end_time(line 814)test_main_invalid_date_format(line 844)Consider either removing the unused parameter or adding assertions to verify the error messages in captured output.
Example fix for test_main_help
- def test_main_help(self, monkeypatch, capsys): + def test_main_help(self, monkeypatch): """Test main with --help flag.""" monkeypatch.setattr("sys.argv", ["make_timelapse.py", "--help"]) with pytest.raises(SystemExit) as exc_info: main() assert exc_info.value.code == 0Or alternatively, use
capsysto verify help content:def test_main_help(self, monkeypatch, capsys): """Test main with --help flag.""" monkeypatch.setattr("sys.argv", ["make_timelapse.py", "--help"]) with pytest.raises(SystemExit) as exc_info: main() assert exc_info.value.code == 0 + captured = capsys.readouterr() + assert "Generate timelapse video" in captured.outAlso applies to: 783-783, 814-814, 844-844
src/make_timelapse.py (3)
25-31: Consider adding type ignore comments for mypy.The fallback import pattern is correct for supporting both installed and development modes, but mypy reports redefinition warnings. You can silence these with type comments if you want the pipeline to pass cleanly.
Optional fix to silence mypy
try: from src.logging_config import get_logger from src.create_keogram import create_keogram_from_images except ModuleNotFoundError: - from logging_config import get_logger - from create_keogram import create_keogram_from_images + from logging_config import get_logger # type: ignore[no-redef] + from create_keogram import create_keogram_from_images # type: ignore[no-redef]
90-93: Add explicit return type or cast for mypy compliance.The function signature declares
-> dictbutyaml.safe_load()returnsAny, causing a mypy error. Consider adding an explicit type annotation or assertion.Suggested fix
-def load_config(config_path: str = "config/config.yml") -> dict: +def load_config(config_path: str = "config/config.yml") -> dict: """Load configuration from YAML file.""" with open(config_path, "r") as f: - return yaml.safe_load(f) + config = yaml.safe_load(f) + if not isinstance(config, dict): + raise ValueError(f"Expected dict in config file, got {type(config)}") + return configOr simply use a type cast if you trust the config format:
from typing import Any, cast def load_config(config_path: str = "config/config.yml") -> dict[str, Any]: """Load configuration from YAML file.""" with open(config_path, "r") as f: return cast(dict[str, Any], yaml.safe_load(f))
478-498: Consider usinglogger.exception()for error logging in except blocks.When logging inside an except block,
logger.exception()automatically includes the traceback, which can be helpful for debugging. Currently,logger.error(str(e))only logs the message.Suggested improvement
try: start_hour, start_min = parse_time(args.start) except ValueError as e: print(Colors.error(f"✗ {e}")) - logger.error(str(e)) + logger.exception("Failed to parse start time") return 1Apply the same pattern to the end time parsing block at line 497.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/tests.ymlsrc/make_timelapse.pytests/test_make_timelapse.py
🧰 Additional context used
🪛 GitHub Actions: Tests
src/make_timelapse.py
[error] 12-12: Library stubs not installed for "yaml" [import-untyped]
[error] 29-29: Name "get_logger" already defined (possibly by an import) [no-redef]
[error] 30-30: Name "create_keogram_from_images" already defined (possibly by an import) [no-redef]
[error] 93-93: Returning Any from function declared to return "dict[Any, Any]" [no-any-return]
🪛 Ruff (0.14.10)
src/make_timelapse.py
223-223: Avoid specifying long messages outside the exception class
(TRY003)
482-482: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
497-497: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
tests/test_make_timelapse.py
761-761: Unused method argument: capsys
(ARG002)
783-783: Unused method argument: capsys
(ARG002)
814-814: Unused method argument: capsys
(ARG002)
844-844: Unused method argument: capsys
(ARG002)
🔇 Additional comments (15)
.github/workflows/tests.yml (1)
10-13: LGTM! Good CI optimization.The concurrency configuration correctly cancels in-progress runs when new commits are pushed to the same branch, saving resources and providing faster feedback. The grouping strategy using
workflowandrefensures runs are properly isolated by branch/PR.tests/test_make_timelapse.py (5)
39-388: LGTM!These test classes provide comprehensive coverage for time parsing, image finding, config loading, integration scenarios, and codec handling. The fixtures properly manage cleanup of temporary resources.
390-441: LGTM!Good coverage of video directory organization patterns with and without date-based subdirectories.
443-519: LGTM!Thorough testing of the
Colorsclass constants and helper methods, as well as the print utility functions with proper output capture.
521-756: LGTM!Excellent coverage of deflicker options, resolution scaling, and error handling scenarios. The tests properly validate both positive and negative cases.
875-937: LGTM!Good coverage of the faststart flag for web streaming and keogram filename extension handling, including the
.jpegto.jpgconversion case.src/make_timelapse.py (9)
33-88: LGTM!Clean implementation of ANSI color utilities and print helpers for consistent terminal output formatting.
96-185: LGTM!Both
parse_timeandfind_images_in_rangeare well-implemented with proper validation and error handling.
188-224: LGTM!Good addition of deflicker parameters with proper validation. The inline exception message (TRY003 hint) is acceptable for a simple validation like this.
270-286: LGTM!Clean modular approach to building the FFmpeg filter chain. The conditional
-vfapplication avoids passing empty filter strings.
356-451: LGTM!Well-structured CLI with comprehensive options for date/time handling, keogram generation, and hardware acceleration. The help text and examples are clear.
506-557: LGTM!The date calculation logic correctly handles overnight timelapses, explicit date arguments, and the
--todayflag. The validation ensures a valid time range.
598-614: LGTM!Clean implementation of hardware encoder and HD resolution flags with appropriate logging and display adjustments.
663-679: LGTM!The filename generation logic creates descriptive filenames that differentiate same-day and multi-day timelapses, preventing accidental overwrites.
700-751: LGTM!Good implementation of keogram generation with proper filename handling and comprehensive status reporting for all output combinations.
1 participant
Description
Type of Change
Checklist
BEFORE SUBMITTING, VERIFY:
make formatorblack src/ tests/ --line-length=100)make testorpython3 -m pytest tests/ -v)make checkorblack --check src/ tests/)Testing
Additional Notes
Did you format your code with Black? ← Most common CI failure!
make format # or: black src/ tests/ --line-length=100Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests & Docs
✏️ Tip: You can customize this high-level summary in your review settings.