Repository navigation
Add type hints, centralize tool config, split Sort.launch - #66
Merged
Merged
Conversation
…unch mypy had no strictness and nothing in src/importrr was annotated, so it was effectively a no-op in CI. Added honest type hints to every module and enabled disallow_untyped_defs per module as each one was typed, then moved ruff/mypy/pytest/coverage config out of mypy.ini and bare CI flags into a single pyproject.toml. Sort.launch had grown into one 105-line method mixing path validation, staging, organizing, metrics, manifest writing, and archiving. Extracted it into _validate_import_dir, _organize_batch, and _archive_batch with no change to call order or the write-ahead-log/cleanup-only-on-success guarantee. Also removed two unused test mock packages left over from before pyexiftool and ffmpy were adopted, normalized test_config.py off unittest.TestCase, added a conftest.py for the previously-duplicated mock_directories fixture, and closed two coverage gaps: launch.py's __main__ guard (extracted into a testable _run()) and sort.py's unreachable IndexError branch (marked pragma: no cover, since string slicing cannot raise it). No runtime behavior changes. Full suite (128 tests) passes, coverage moved from 98.76% to 99.85%, ruff and mypy are clean.
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Project tooling configuration mypy.ini, pyproject.toml |
Moves mypy settings into pyproject.toml, adds per-module typing requirements and Ruff rules, and removes mypy.ini. |
Application type annotations src/importrr/archive.py, src/importrr/config.py, src/importrr/exifhelper.py, src/importrr/metrics.py, src/importrr/transcode.py |
Adds parameter, return, and local variable annotations. The summaries report no executable behavior changes in these modules. |
Sorting pipeline organization and archiving src/importrr/sort.py |
Adds annotations and separates import-path validation, batch organization, and archiving into methods used by launch. Archive errors are counted and re-raised. |
Startup entry point and tests src/importrr/launch.py, tests/test_launch.py |
Moves startup statements into _run(). Adds tests for metrics startup with default, configured, and disabled settings; each test expects the scheduler to start. |
Pytest configuration and fixtures pyproject.toml, tests/conftest.py, tests/mocks/*, tests/test_config.py, tests/test_sort_security.py |
Configures pytest and coverage, adds a shared directory fixture, converts configuration checks to pytest functions, and removes test mocks and the local security-test fixture definition. |
Priority:
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Refactor
Possibly related PRs
- curfew-marathon/importrr#50: The sorting refactor relocates metric updates and archive-error handling added in that PR.
- curfew-marathon/importrr#55: The sorting refactor relocates unprocessed-file classification and manifest writing added in that PR.
- curfew-marathon/importrr#65: This pull request annotates the previously noted unreachable sorting branch and moves the main-guard startup logic into the tested
_run()entry point.
Merge Risk: 
bbe24
-
No actionable regression is identified; the change appears mergeable after normal checks.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 39.58% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 48 functions across 10 files. (1 skipped:… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly summarizes the main changes: adding type hints, centralizing tool configuration, and splitting Sort.launch.Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 39.58% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 48 functions across 10 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
There was a problem hiding this comment.
🟡 Changes recommended
The missing mypy source-package base prevents the new strictness overrides from applying in CI and weakens cross-module checks.
1 open issue
What changed in this PR
This PR adds type annotations and centralized tooling configuration to importrr, while splitting batch processing into smaller helpers intended to preserve existing behavior.
Changes:
- Annotates source functions and adds module-specific mypy strictness settings.
- Extracts sorting phases and scheduler startup into focused helpers.
- Consolidates test fixtures, adds startup tests, and removes unused mocks.
| File | Description |
|---|---|
tests/test_sort_security.py |
Uses the shared directory fixture. |
tests/test_launch.py |
Tests metrics configuration and scheduler startup. |
tests/test_config.py |
Converts unittest methods to pytest functions. |
tests/mocks/ffmpy/__init__.py |
Removes unused FFmpeg mocks. |
tests/mocks/exiftool/exceptions.py |
Removes an unused exception mock. |
tests/mocks/exiftool/__init__.py |
Removes unused ExifTool mocks. |
tests/conftest.py |
Provides the shared directory fixture. |
src/importrr/transcode.py |
Adds annotations and logging type suppressions. |
src/importrr/sort.py |
Adds annotations and extracts batch-processing helpers. |
src/importrr/metrics.py |
Annotates metrics startup and documents invalid-port handling. |
src/importrr/launch.py |
Adds annotations and extracts testable startup logic. |
src/importrr/exifhelper.py |
Annotates ExifTool helpers and makes a fallback return explicit. |
src/importrr/config.py |
Annotates configuration methods and stored data. |
src/importrr/archive.py |
Adds annotations and clarifies error-handling comments. |
pyproject.toml |
Centralizes mypy, Ruff, pytest, and coverage settings. |
mypy.ini |
Removes configuration migrated to pyproject.toml. |
Review effort: Balanced
CI runs mypy . from the repo root, which resolved every src/importrr module as src.importrr.X instead of importrr.X (confirmed via mypy --verbose). That meant every [[tool.mypy.overrides]] module = "importrr.X" block added in the prior refactor matched nothing, and internal "from importrr import ..." statements were falling back to ignore_missing_imports, silently stubbing cross-module calls as Any. Adding mypy_path = "src" fixes the module resolution and makes mypy actually check cross-module calls, which surfaced two real (if currently harmless) gaps in launch.py's main_process: Config.get_data() entries are typed dict[str, Any], so d.get(...) is Optional even for keys Config always populates. Suppressed with scoped type: ignore comments rather than switching to dict indexing, since that would change the exception type logged in the hypothetical case of a missing key. Addresses the Copilot review comment on PR #66.
curfew-marathon
added a commit
that referenced
this pull request
Oct 25, 2026
* Add type hints across importrr, centralize tool config, split Sort.launch mypy had no strictness and nothing in src/importrr was annotated, so it was effectively a no-op in CI. Added honest type hints to every module and enabled disallow_untyped_defs per module as each one was typed, then moved ruff/mypy/pytest/coverage config out of mypy.ini and bare CI flags into a single pyproject.toml. Sort.launch had grown into one 105-line method mixing path validation, staging, organizing, metrics, manifest writing, and archiving. Extracted it into _validate_import_dir, _organize_batch, and _archive_batch with no change to call order or the write-ahead-log/cleanup-only-on-success guarantee. Also removed two unused test mock packages left over from before pyexiftool and ffmpy were adopted, normalized test_config.py off unittest.TestCase, added a conftest.py for the previously-duplicated mock_directories fixture, and closed two coverage gaps: launch.py's __main__ guard (extracted into a testable _run()) and sort.py's unreachable IndexError branch (marked pragma: no cover, since string slicing cannot raise it). No runtime behavior changes. Full suite (128 tests) passes, coverage moved from 98.76% to 99.85%, ruff and mypy are clean. * Fix mypy_path so the per-module strictness overrides actually apply CI runs mypy . from the repo root, which resolved every src/importrr module as src.importrr.X instead of importrr.X (confirmed via mypy --verbose). That meant every [[tool.mypy.overrides]] module = "importrr.X" block added in the prior refactor matched nothing, and internal "from importrr import ..." statements were falling back to ignore_missing_imports, silently stubbing cross-module calls as Any. Adding mypy_path = "src" fixes the module resolution and makes mypy actually check cross-module calls, which surfaced two real (if currently harmless) gaps in launch.py's main_process: Config.get_data() entries are typed dict[str, Any], so d.get(...) is Optional even for keys Config always populates. Suppressed with scoped type: ignore comments rather than switching to dict indexing, since that would change the exception type logged in the hypothetical case of a missing key. Addresses the Copilot review comment on PR #66. * Pin Docker base image and align Python target to 3.15 The Dockerfile used FROM python:alpine with no tag, which silently floats to whatever Python release is current. A digest comparison showed it already resolved to Python 3.15, three minors ahead of the 3.11 that CI, mypy, and the project docs all claimed. Pin the base image to what it already runs today (preserves current behavior, no downgrade), and bring CI's setup-python, mypy's python_version, ruff's target-version, and CLAUDE.md in line with that same version so they stop silently drifting apart. Also raise the enforced coverage floor to 99%, matching actual coverage, since it had been left at a stale 80%.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
if __name__ == "__main__":guard (extracted into a testable_run(), with new tests) and sort.py'sexcept IndexError:branch in sort_media, which is actually unreachable (Python string slicing never raises IndexError) and is now marked# pragma: no coverwith a comment explaining why, instead of being left as a silent gap.No runtime behavior changes anywhere in this PR. The one open item identified during this work (launch.py's signal handler calling sys.exit(0) after scheduler.shutdown(), which could in principle skip Sort.launch's cleanup if a signal lands mid-batch) is deliberately not touched here since fixing it would be a behavior change; flagging it as a follow-up for a separate PR if wanted.
Test plan
ruff check .andruff format --check .passmypy .passes with no errors across all 18 source filesPYTHONPATH=src pytest --cov=src --cov-report=term-missing --cov-fail-under=80passes: 128 tests (3 new), coverage 99.85% (up from 98.76% baseline), floor unchanged at 80%git diff -U20 src/importrr/sort.pyto confirm every statement in the original Sort.launch body appears exactly once, same relative order, after the extractionSummary by CodeRabbit