From c12d5b6d82859f29c4fbf4efd6ce9b955e6a311b Mon Sep 17 00:00:00 2001 From: Ed_ Date: Fri, 26 Jun 2026 05:01:15 -0400 Subject: [PATCH] refactor(models,paths,presets,summary_cache): remove Optional returns (Phase 6 batch 1) Phase 6: Eliminate Optional[T] returns (FR5) - BATCH 1 of 7 Before: 8 Optional[T] return types across 4 files After: 0 (replaced with default-zero return values) Delta: -8 sites Per conductor/code_styleguides/error_handling.md "Optional[X] ban": - "Use Result[T] for any function that can fail at runtime." - "Use nil-sentinel dataclasses for 'no result'." For accessor-style returns (lookup or zero-default), convert to: - Optional[str] -> str with default "" (empty string sentinel) - Optional[float] -> float with default 0.0 - Optional[int] -> int with default 0 - Optional[Path] -> Path with default Path("") or project_root Specific changes: - src/models.py:765-789: Persona.provider/model/temperature/top_p/max_output_tokens (Optional[str]/[float]/[int] -> str/float/int with default zero values) - src/paths.py:255: _get_project_conductor_dir_from_toml returns project_root when no [conductor].dir override is configured (was Optional[Path] returning None) - src/presets.py:21: project_path property returns Path("") when no project_root (was Optional[Path] returning None) - src/summary_cache.py:57: get_summary returns "" when hash mismatch (was Optional[str] returning None) Test updates: - tests/test_persona_models.py:64-69: test_persona_defaults now expects "" / 0.0 instead of None - tests/test_summary_cache.py:25, 32, 58: get_summary assertions now expect "" instead of None Verification: - audit_weak_types --strict: OK (107 <= 112 baseline) - 13 tests pass (test_summary_cache, test_paths, test_presets, test_persona_models) - py_check_syntax: OK on all changed files REMAINING: ~22 Optional[T] returns in: - src/command_palette.py (1) - src/diff_viewer.py (2) - src/external_editor.py (3) - src/file_cache.py (7) - src/fuzzy_anchor.py (1) - src/models.py (1) - src/multi_agent_conductor.py (1) - src/patch_modal.py (1) - src/project_manager.py (1) - src/session_logger.py (1) - src/app_controller.py (3) --- src/models.py | 30 +++++++++++++++--------------- src/paths.py | 14 ++++++++------ src/presets.py | 4 ++-- src/summary_cache.py | 8 ++++---- tests/test_persona_models.py | 6 +++--- tests/test_summary_cache.py | 8 ++++---- 6 files changed, 36 insertions(+), 34 deletions(-) diff --git a/src/models.py b/src/models.py index e1059afd..ad8e403c 100644 --- a/src/models.py +++ b/src/models.py @@ -762,29 +762,29 @@ class Persona: aggregation_strategy: Optional[str] = None @property - def provider(self) -> Optional[str]: - if not self.preferred_models: return None - return self.preferred_models[0].get("provider") + def provider(self) -> str: + if not self.preferred_models: return "" + return self.preferred_models[0].get("provider") or "" @property - def model(self) -> Optional[str]: - if not self.preferred_models: return None - return self.preferred_models[0].get("model") + def model(self) -> str: + if not self.preferred_models: return "" + return self.preferred_models[0].get("model") or "" @property - def temperature(self) -> Optional[float]: - if not self.preferred_models: return None - return self.preferred_models[0].get("temperature") + def temperature(self) -> float: + if not self.preferred_models: return 0.0 + return float(self.preferred_models[0].get("temperature") or 0.0) @property - def top_p(self) -> Optional[float]: - if not self.preferred_models: return None - return self.preferred_models[0].get("top_p") + def top_p(self) -> float: + if not self.preferred_models: return 1.0 + return float(self.preferred_models[0].get("top_p") or 1.0) @property - def max_output_tokens(self) -> Optional[int]: - if not self.preferred_models: return None - return self.preferred_models[0].get("max_output_tokens") + def max_output_tokens(self) -> int: + if not self.preferred_models: return 0 + return int(self.preferred_models[0].get("max_output_tokens") or 0) def to_dict(self) -> Metadata: """ diff --git a/src/paths.py b/src/paths.py index 7d494e5f..506ac258 100644 --- a/src/paths.py +++ b/src/paths.py @@ -252,10 +252,11 @@ def get_archive_dir(project_path: Optional[str] = None) -> Path: return get_conductor_dir(project_path) / "archive" -def _get_project_conductor_dir_from_toml(project_root: Path) -> Optional[Path]: - """Look for manual_slop.toml in project_root for [conductor] dir override.""" +def _get_project_conductor_dir_from_toml(project_root: Path) -> Path: + """Look for manual_slop.toml in project_root for [conductor] dir override. + Returns the resolved Path, or project_root if no override configured.""" toml_path = project_root / 'manual_slop.toml' - if not toml_path.exists(): return None + if not toml_path.exists(): return project_root try: with open(toml_path, 'rb') as f: data = tomllib.load(f) @@ -265,7 +266,7 @@ def _get_project_conductor_dir_from_toml(project_root: Path) -> Optional[Path]: if not p.is_absolute(): p = project_root / p return p.resolve() except: pass - return None + return project_root def get_conductor_dir(project_path: Optional[str] = None) -> Path: @@ -273,8 +274,9 @@ def get_conductor_dir(project_path: Optional[str] = None) -> Path: if not project_path: return Path('conductor').resolve() project_root = Path(project_path).resolve() - p = _get_project_conductor_dir_from_toml(project_root) - if p: return p + toml_path = project_root / 'manual_slop.toml' + if toml_path.exists(): + return _get_project_conductor_dir_from_toml(project_root) return (project_root / "conductor").resolve() diff --git a/src/presets.py b/src/presets.py index 6f64ea3a..679848b5 100644 --- a/src/presets.py +++ b/src/presets.py @@ -18,8 +18,8 @@ class PresetManager: self.global_path = get_global_presets_path() @property - def project_path(self) -> Optional[Path]: - return get_project_presets_path(self.project_root) if self.project_root else None + def project_path(self) -> Path: + return get_project_presets_path(self.project_root) if self.project_root else Path("") def load_all(self) -> Dict[str, Preset]: """ diff --git a/src/summary_cache.py b/src/summary_cache.py index b68f8c27..a409abe5 100644 --- a/src/summary_cache.py +++ b/src/summary_cache.py @@ -54,9 +54,9 @@ class SummaryCache: except OSError as e: return Result(data=False, errors=[ErrorInfo(kind=ErrorKind.INTERNAL, message=str(e), source="summary_cache.save", original=e)]) - def get_summary(self, file_path: str, content_hash: str) -> Optional[str]: + def get_summary(self, file_path: str, content_hash: str) -> str: """ - Returns cached summary if hash matches, otherwise None. + Returns cached summary if hash matches, otherwise "". [C: tests/test_summary_cache.py:test_summary_cache, tests/test_summary_cache.py:test_summary_cache_lru] """ entry = self.cache.get(file_path) @@ -64,8 +64,8 @@ class SummaryCache: # LRU: move to end val = self.cache.pop(file_path) self.cache[file_path] = val - return val.get("summary") - return None + return val.get("summary") or "" + return "" def set_summary(self, file_path: str, content_hash: str, summary: str) -> None: """ diff --git a/tests/test_persona_models.py b/tests/test_persona_models.py index bbd30fa5..3a91896f 100644 --- a/tests/test_persona_models.py +++ b/tests/test_persona_models.py @@ -65,10 +65,10 @@ def test_persona_deserialization(): def test_persona_defaults(): persona = Persona(name="Minimal", system_prompt="Just the basics") - assert persona.provider is None - assert persona.model is None + assert persona.provider == "" + assert persona.model == "" assert persona.preferred_models == [] - assert persona.temperature is None + assert persona.temperature == 0.0 assert persona.tool_preset is None data = persona.to_dict() diff --git a/tests/test_summary_cache.py b/tests/test_summary_cache.py index 99df6967..3cb5e5cf 100644 --- a/tests/test_summary_cache.py +++ b/tests/test_summary_cache.py @@ -22,14 +22,14 @@ def test_summary_cache(tmp_path): summary = "**Python** - 1 lines" # Test empty cache - assert cache.get_summary(file_path, content_hash) is None + assert cache.get_summary(file_path, content_hash) == "" # Test set and get cache.set_summary(file_path, content_hash, summary) assert cache.get_summary(file_path, content_hash) == summary # Test cache invalidation - assert cache.get_summary(file_path, "different_hash") is None + assert cache.get_summary(file_path, "different_hash") == "" # Test persistence cache2 = SummaryCache(str(cache_file)) @@ -47,7 +47,7 @@ def test_summary_cache_lru(tmp_path): cache.set_summary("file2.py", "hash2", "summary2") cache.set_summary("file3.py", "hash3", "summary3") # This should evict file1.py - assert cache.get_summary("file1.py", "hash1") is None + assert cache.get_summary("file1.py", "hash1") == "" assert cache.get_summary("file2.py", "hash2") == "summary2" assert cache.get_summary("file3.py", "hash3") == "summary3" @@ -55,7 +55,7 @@ def test_summary_cache_lru(tmp_path): cache.get_summary("file2.py", "hash2") cache.set_summary("file4.py", "hash4", "summary4") - assert cache.get_summary("file3.py", "hash3") is None + assert cache.get_summary("file3.py", "hash3") == "" assert cache.get_summary("file2.py", "hash2") == "summary2" assert cache.get_summary("file4.py", "hash4") == "summary4"