Fix duplicate DialogManager methods and make tooling Python 3.14-aware#48
Conversation
DialogManager defined show_transfer_stats and show_disable_debugging_reminder
twice; the later definitions silently shadowed the earlier ones. The live
show_transfer_stats(self, stats, operation) accepted only two arguments, but
TransferManager passed a third (deduplicator), raising TypeError inside the Tk
after() callback so the transfer-stats window never appeared after folder
transfers. The shadowed copies were also dead: they indexed stats keys
(bytes_saved/total_files/...) that the *_with_dedup TODO stubs never produce,
so they would have KeyError'd on the real {"message": ...} payload.
- Remove the dead, shadowed copies of both methods
- Drop the unused deduplicator argument at the caller (transfer_manager)
- Annotate the surviving methods with -> None
Also align linting/type config with the existing >=3.13,<3.15 support that CI
already exercises: black now targets py313+py314, and CLAUDE.md documents
3.13/3.14. mypy stays pinned to the 3.13 floor (the oldest supported runtime).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Code review posted below - please see the review summary. |
|
Code Review — PR 48: Fix duplicate DialogManager methods and make tooling Python 3.14-aware Overview Fixes a silent runtime bug from duplicate DialogManager methods and syncs tooling/docs to actual Python 3.14 support. Scope is appropriately narrow — no new features, only correctness and cleanup. Bug Fix: Duplicate DialogManager Methods This is a genuine fix. Python silently shadows earlier method definitions when two share the same name. The 3-arg show_transfer_stats was masked by the later 2-arg version, yet TransferManager called it with 3 positional args — a guaranteed TypeError inside a Tk.after() callback that was swallowed silently, causing the stats window to never appear after folder transfers. Removing the shadowed copy and dropping the extra arg at the call site is the right minimal fix. Concern — stats formatting regression: The removed show_transfer_stats had richer output (total_files, transferred, skipped, bytes_saved fields). The surviving version renders a generic message dump. The PR correctly explains why (the with_dedup stubs only return a generic message dict so the structured fields would KeyError anyway). That reasoning is sound. But consider a TODO comment in the surviving method so the structured rendering is not forgotten once the dedup stubs land. Concern — deduplicator: Removing the deduplicator arg from the caller is correct now, but the dialog will need it back for byte formatting once the dedup stubs are implemented. Worth a follow-up issue or TODO note. Python 3.14 Tooling Sync
Missing Test Coverage No tests are added or modified. Given this fixes a silent runtime failure, regression tests would be valuable:
These would have caught the original bug and guard against reintroducing it. Minor
Summary
Overall a clean, well-scoped fix. Main asks: (1) add a TODO in show_transfer_stats so the structured stats rendering is not lost when the with_dedup stubs are implemented, and (2) add at least one regression test for the dialog call signature. |
Summary
Two fixes, both confined to existing-state cleanup (no new features).
1. Duplicate
DialogManagermethods → runtimeTypeErrorshow_transfer_statsandshow_disable_debugging_reminderwere each defined twice; the later definitions silently shadowed the earlier ones. The liveshow_transfer_stats(self, stats, operation)took only two args, butTransferManagerpassed a third (deduplicator), raisingTypeErrorinside the Tkafter()callback — so the transfer-stats window never appeared after folder transfers (it failed silently with a console traceback).The shadowed copies were also dead code: they indexed
stats["bytes_saved"]/["total_files"]/etc. — keys the*_with_dedupTODO stubs inadb_manager.pynever produce (they return{"message": ...}), so they'd haveKeyError'd on real data.deduplicatorargument at the caller-> NoneBehavior change: the transfer-stats window will now actually appear after folder transfers, rendering
Message: <…>(generic dump). The richer structured summary depends on implementing the*_with_dedupTODO and is out of scope here.2. Python 3.14 tooling/doc sync
The project already declares
requires-python = ">=3.13, <3.15"and CI tests['3.13','3.14'], but black/docs ignored 3.14.black target-version→['py313', 'py314']CLAUDE.md→ "Requires Python 3.13 or 3.14 (>=3.13, <3.15)"mypyintentionally kept at the 3.13 floor (oldest supported runtime)Verification
python -m src.main, no traceback)no-redeffor both methods gone🤖 Generated with Claude Code