From 5e599b9e7d65dab3f6b2133931f37b58ba390062 Mon Sep 17 00:00:00 2001 From: Sven Lito Date: Sun, 10 Aug 2025 22:13:25 +0700 Subject: [PATCH] Complete PR review feedback implementation with clean importlib.resources approach - Remove redundant path checks between Path("conf/custom_models.json") and Path.cwd() variants - Implement proper importlib.resources.files('conf') approach for robust packaging - Create conf/__init__.py to make conf a proper Python package - Update pyproject.toml to include conf* in package discovery - Clean up verbose comments and simplify resource loading logic - Fix test mocking to use correct importlib.resources.files target - All tests passing (8/8) with proper resource and fallback functionality Addresses all gemini-code-assist bot feedback from PR #227 --- conf/__init__.py | 1 + providers/openrouter_registry.py | 36 +++++++++++++--------------- pyproject.toml | 2 +- tests/test_uvx_resource_packaging.py | 4 ++-- 4 files changed, 20 insertions(+), 23 deletions(-) create mode 100644 conf/__init__.py diff --git a/conf/__init__.py b/conf/__init__.py new file mode 100644 index 0000000..ee1bfd4 --- /dev/null +++ b/conf/__init__.py @@ -0,0 +1 @@ +"""Configuration data for Zen MCP Server.""" diff --git a/providers/openrouter_registry.py b/providers/openrouter_registry.py index b21361a..0024a1a 100644 --- a/providers/openrouter_registry.py +++ b/providers/openrouter_registry.py @@ -1,16 +1,12 @@ """OpenRouter model registry for managing model configurations and aliases.""" +import importlib.resources import logging import os from pathlib import Path from typing import Optional -try: - from importlib.resources import files -except ImportError: - # Python < 3.9 fallback - from importlib_resources import files - +# Import handled via importlib.resources.files() calls directly from utils.file_utils import read_json_file from .base import ( @@ -44,31 +40,31 @@ class OpenRouterModelRegistry: # Environment variable path self.config_path = Path(env_path) else: - # Try importlib.resources first (proper way for packaged environments) + # Try importlib.resources for robust packaging support + self.config_path = None + self.use_resources = False + try: - # Test if we can access the config via resources - resource_path = files("providers") / "../conf/custom_models.json" - if hasattr(resource_path, "read_text"): - # Resource exists and is accessible - self.config_path = None + resource_traversable = importlib.resources.files('conf').joinpath('custom_models.json') + if hasattr(resource_traversable, 'read_text'): self.use_resources = True else: - raise FileNotFoundError("Resource method not available") + raise AttributeError("read_text not available") except Exception: - # Fall back to file system paths + pass + + if not self.use_resources: + # Fallback to file system paths potential_paths = [ - Path(__file__).parent.parent / "conf" / "custom_models.json", # Development - Path("conf/custom_models.json"), # Working directory + Path(__file__).parent.parent / "conf" / "custom_models.json", + Path.cwd() / "conf" / "custom_models.json", ] - # Find first existing path - self.config_path = None for path in potential_paths: if path.exists(): self.config_path = path break - # If none found, default to the first one for error reporting if self.config_path is None: self.config_path = potential_paths[0] @@ -131,7 +127,7 @@ class OpenRouterModelRegistry: if self.use_resources: # Use importlib.resources for packaged environments try: - resource_path = files("providers") / "../conf/custom_models.json" + resource_path = importlib.resources.files('conf').joinpath('custom_models.json') if hasattr(resource_path, "read_text"): # Python 3.9+ config_text = resource_path.read_text(encoding="utf-8") diff --git a/pyproject.toml b/pyproject.toml index b3e715b..22b67cf 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -12,7 +12,7 @@ dependencies = [ ] [tool.setuptools.packages.find] -include = ["tools*", "providers*", "systemprompts*", "utils*"] +include = ["tools*", "providers*", "systemprompts*", "utils*", "conf*"] [tool.setuptools] py-modules = ["server", "config"] diff --git a/tests/test_uvx_resource_packaging.py b/tests/test_uvx_resource_packaging.py index e64d3a9..f83f144 100644 --- a/tests/test_uvx_resource_packaging.py +++ b/tests/test_uvx_resource_packaging.py @@ -53,7 +53,7 @@ class TestUvxPathResolution: assert registry.config_path == config_path assert len(registry.list_models()) > 0 - @patch("providers.openrouter_registry.files") + @patch("providers.openrouter_registry.importlib.resources.files") @patch("pathlib.Path.exists") def test_multiple_path_fallback(self, mock_exists, mock_files): """Test that multiple path resolution works for different deployment scenarios.""" @@ -70,7 +70,7 @@ class TestUvxPathResolution: assert not registry.use_resources, "Should fall back to file system when resources fail" # Assert that the registry fell back to the second potential path - assert registry.config_path == Path("conf/custom_models.json") + assert registry.config_path == Path.cwd() / "conf" / "custom_models.json" # Should load models successfully assert len(registry.list_models()) > 0