Skip to content

Add comprehensive type annotations to scripts module#2979

Merged
jonathangreen merged 2 commits intomainfrom
chore/strict-typing-scripts-base
Jan 15, 2026
Merged

Add comprehensive type annotations to scripts module#2979
jonathangreen merged 2 commits intomainfrom
chore/strict-typing-scripts-base

Conversation

@jonathangreen
Copy link
Member

@jonathangreen jonathangreen commented Jan 11, 2026

Description

Adds complete type annotations to the scripts module, removing 25+ modules from the mypy ignore list. This enables full type checking for all scripts, improving code quality and maintainability.

Key Changes:

  • Added type annotations to all script base classes and input scripts
  • Centralized _normalize_cmd_args() helper to eliminate code duplication
  • Added from __future__ import annotations for modern type syntax
  • Replaced generic exceptions with PalaceValueError

Motivation and Context

The scripts module was excluded from mypy type checking. This PR brings it up to the same type safety standards as the rest of the codebase, catching type errors at development time instead of runtime.

How Has This Been Tested?

  • mypy: All 28 script files pass type checking
  • pytest: 130 script tests pass with no errors
  • pre-commit: All formatting and linting checks pass

Checklist

  • I have updated the documentation accordingly.
  • All new and existing tests passed.

@codecov
Copy link

codecov bot commented Jan 11, 2026

Codecov Report

❌ Patch coverage is 79.78723% with 95 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.79%. Comparing base (42a82b2) to head (9358491).
⚠️ Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
src/palace/manager/scripts/informational.py 66.66% 21 Missing and 1 partial ⚠️
src/palace/manager/scripts/availability.py 0.00% 13 Missing ⚠️
src/palace/manager/scripts/metadata.py 0.00% 12 Missing ⚠️
src/palace/manager/scripts/work.py 70.58% 10 Missing ⚠️
src/palace/manager/scripts/monitor.py 70.83% 6 Missing and 1 partial ⚠️
src/palace/manager/scripts/translations.py 0.00% 5 Missing ⚠️
.../palace/manager/integration/license/bibliotheca.py 33.33% 4 Missing ⚠️
src/palace/manager/scripts/lane.py 81.81% 3 Missing and 1 partial ⚠️
src/palace/manager/scripts/configuration.py 91.89% 2 Missing and 1 partial ⚠️
src/palace/manager/scripts/identifier.py 81.25% 2 Missing and 1 partial ⚠️
... and 7 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2979      +/-   ##
==========================================
- Coverage   92.81%   92.79%   -0.02%     
==========================================
  Files         454      454              
  Lines       42954    43095     +141     
  Branches     6000     6017      +17     
==========================================
+ Hits        39866    39992     +126     
- Misses       2018     2032      +14     
- Partials     1070     1071       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jonathangreen jonathangreen requested a review from a team January 11, 2026 14:42
@jonathangreen jonathangreen force-pushed the chore/strict-typing-scripts-base branch 2 times, most recently from 46dbf87 to ef38be6 Compare January 11, 2026 16:34
@jonathangreen jonathangreen removed the request for review from a team January 12, 2026 13:47
@jonathangreen jonathangreen marked this pull request as draft January 12, 2026 13:47
@jonathangreen
Copy link
Member Author

I'm going to move this back to draft, I need to look into why the coverage dropped here before it gets reviewed.

Add comprehensive type annotations to all script classes in palace.manager.scripts
and related integration modules. Remove 24 script modules from mypy exclusion list
to enable strict type checking.

Key changes:
- Add type hints to all function signatures and class methods
- Update arg_parser() methods to accept _db: Session parameter
- Migrate from generic exceptions to Palace-specific exceptions
- Modernize code with f-strings and improved error messages
- Fix availability.py to work with updated API signatures requiring collection parameter

Breaking change: arg_parser() methods now require _db parameter for consistency
across all script classes.
@jonathangreen jonathangreen force-pushed the chore/strict-typing-scripts-base branch from dd25ca1 to f9a2f46 Compare January 12, 2026 15:16
Add comprehensive test coverage for script error paths and initialization logic:
- Add test for ShowLibrariesScript with nonexistent library name
- Add test for ShowCollectionsScript with nonexistent collection name
- Add tests for WorkProcessingScript.__init__ covering all initialization paths
- Fix missing newline in ShowCollectionsScript error message for consistency

The WorkProcessingScript tests cover:
- Default initialization with no arguments
- Initialization with identifier type filtering
- Error validation when identifier strings provided without type
- Initialization with specific identifier IDs
- Custom batch_size and force parameters
- Data source filtering
@jonathangreen jonathangreen requested a review from a team January 12, 2026 15:59
@jonathangreen jonathangreen marked this pull request as ready for review January 12, 2026 15:59
@jonathangreen
Copy link
Member Author

Okay, I added a few additional test. The rest of the test coverage reduction is are some of the defensive checks that strict type hints necessitated. I think the patch coverage is all pre-existing coverage gaps.

Copy link
Contributor

@dbernstein dbernstein left a comment

Choose a reason for hiding this comment

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

Looks good.

@jonathangreen jonathangreen merged commit a89c7a3 into main Jan 15, 2026
18 of 19 checks passed
@jonathangreen jonathangreen deleted the chore/strict-typing-scripts-base branch January 15, 2026 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants