Add type annotations to borg layer modules (Phase 3 of #2362)#2453
Add type annotations to borg layer modules (Phase 3 of #2362)#2453ratnadeveloper wants to merge 4 commits intoborgbase:masterfrom
Conversation
Following the conventions established in PR borgbase#2402: - Add from __future__ import annotations to all borg modules - Add type hints to all function signatures (parameters and return types) - No redundant variable annotations (only function signatures) - Use built-in generics (dict, list, tuple) via PEP 604/585 Files annotated (21 files): - borg_job.py: Base class with __init__, prepare, run, process_result, etc. - jobs_manager.py: JobInterface, SiteWorker, JobsManager - _compatibility.py: BorgCompatibility class - All subcommand modules: break_lock, change_passphrase, check, compact, create, delete, diff, extract, info_archive, info_repo, init, list_archive, list_repo, mount, prune, rename, umount, version Related to borgbase#2362
m3nu
left a comment
There was a problem hiding this comment.
Review
profile: Any should use the actual type
Every prepare() method annotates profile as Any. The actual type is BackupProfileModel (or FakeProfile in init/info_repo). Both share the same duck-type interface (.repo, .ssh_key, .name, .id). Using Any defeats the purpose — mypy won't catch attribute typos or misuse. A proper annotation would be BackupProfileModel, a Protocol, or Union[BackupProfileModel, FakeProfile].
No mypy enforcement — annotations are decorative
The mypy config in pyproject.toml has disallow_untyped_defs = false globally, and the borg layer has no override. These annotations will never be checked. The PR should include:
[[tool.mypy.overrides]]
module = ["vorta.borg.*"]
disallow_untyped_defs = trueWithout this, annotations can silently drift from reality.
Inconsistency with existing typed files
change_passphrase.py, check.py, and compact.py already had from typing import Any, Dict and partial annotations (e.g., finished_event(self, result: Dict[str, Any])). The PR adds from __future__ import annotations but does not:
- Update existing
Dict[str, Any]→dict[str, Any](lowercase builtin) - Annotate the remaining untyped methods (
started_event,prepare,process_result)
This leaves files in a mixed state.
Dead import in delete.py
The signature was changed from List[str] to list[str], but List is still imported from typing. It's now dead code.
-> None annotations are inferable
For methods like started_event, finished_event, cancel, run — mypy infers -> None when a function has no return statement. These annotations aren't wrong, but they add noise without catching new bugs. The parameter types and non-obvious return types (prepare_bin() -> str | None) are the ones that matter.
dict[str, Any] return type for prepare() is imprecise
Every prepare() returns a dict with a known structure (ok, message, cmd, repo_id, repo_url, ssh_key, etc.). A TypedDict would provide actual safety. dict[str, Any] tells you it's a dict but nothing about its keys — marginally better than no annotation.
Import ordering in borg_job.py
from typing import Any is placed between from collections import namedtuple and from datetime import datetime as dt, breaking alphabetical stdlib import ordering.
- Replace profile: Any with BackupProfileModel (or BackupProfileModel | FakeProfile for init/info_repo) - Add mypy override for vorta.borg.* (disallow_untyped_defs = true) - Update Dict[str, Any] to dict[str, Any] in check.py, compact.py, change_passphrase.py - Annotate remaining untyped methods in check.py, compact.py, change_passphrase.py - Remove dead List import from delete.py - Remove noisy -> None from methods where mypy infers it (started_event, finished_event, cancel, run, __init__) - Fix import ordering in borg_job.py (alphabetical stdlib imports)
…ratnadeveloper/vorta into add-type-annotations-borg-layer
|
Thank you for the thorough review @m3nu! I've addressed all 6 points:
|
2 similar comments
|
Thank you for the thorough review @m3nu! I've addressed all 6 points:
|
|
Thank you for the thorough review @m3nu! I've addressed all 6 points:
|
|
Thank you for the thorough review @m3nu! I've addressed all 6 points:
Please let me know if anything else needs adjustment! |
Description
Phase 3 of #2362 — adds comprehensive type annotations to all borg layer modules, following the conventions established in PR #2402.
Changes
from __future__ import annotationsto all 21 borg modulesdict,list,tuple) via PEP 604/585Files annotated (21 files)
borg_job.py— Base BorgJob classjobs_manager.py— JobInterface, SiteWorker, JobsManager_compatibility.py— BorgCompatibilityConventions followed (per #2402 review)
from __future__ import annotationsin every module (PEP 563)Related to #2362