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
This commit is contained in:
1
conf/__init__.py
Normal file
1
conf/__init__.py
Normal file
@@ -0,0 +1 @@
|
|||||||
|
"""Configuration data for Zen MCP Server."""
|
||||||
@@ -1,16 +1,12 @@
|
|||||||
"""OpenRouter model registry for managing model configurations and aliases."""
|
"""OpenRouter model registry for managing model configurations and aliases."""
|
||||||
|
|
||||||
|
import importlib.resources
|
||||||
import logging
|
import logging
|
||||||
import os
|
import os
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
from typing import Optional
|
from typing import Optional
|
||||||
|
|
||||||
try:
|
# Import handled via importlib.resources.files() calls directly
|
||||||
from importlib.resources import files
|
|
||||||
except ImportError:
|
|
||||||
# Python < 3.9 fallback
|
|
||||||
from importlib_resources import files
|
|
||||||
|
|
||||||
from utils.file_utils import read_json_file
|
from utils.file_utils import read_json_file
|
||||||
|
|
||||||
from .base import (
|
from .base import (
|
||||||
@@ -44,31 +40,31 @@ class OpenRouterModelRegistry:
|
|||||||
# Environment variable path
|
# Environment variable path
|
||||||
self.config_path = Path(env_path)
|
self.config_path = Path(env_path)
|
||||||
else:
|
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:
|
try:
|
||||||
# Test if we can access the config via resources
|
resource_traversable = importlib.resources.files('conf').joinpath('custom_models.json')
|
||||||
resource_path = files("providers") / "../conf/custom_models.json"
|
if hasattr(resource_traversable, 'read_text'):
|
||||||
if hasattr(resource_path, "read_text"):
|
|
||||||
# Resource exists and is accessible
|
|
||||||
self.config_path = None
|
|
||||||
self.use_resources = True
|
self.use_resources = True
|
||||||
else:
|
else:
|
||||||
raise FileNotFoundError("Resource method not available")
|
raise AttributeError("read_text not available")
|
||||||
except Exception:
|
except Exception:
|
||||||
# Fall back to file system paths
|
pass
|
||||||
|
|
||||||
|
if not self.use_resources:
|
||||||
|
# Fallback to file system paths
|
||||||
potential_paths = [
|
potential_paths = [
|
||||||
Path(__file__).parent.parent / "conf" / "custom_models.json", # Development
|
Path(__file__).parent.parent / "conf" / "custom_models.json",
|
||||||
Path("conf/custom_models.json"), # Working directory
|
Path.cwd() / "conf" / "custom_models.json",
|
||||||
]
|
]
|
||||||
|
|
||||||
# Find first existing path
|
|
||||||
self.config_path = None
|
|
||||||
for path in potential_paths:
|
for path in potential_paths:
|
||||||
if path.exists():
|
if path.exists():
|
||||||
self.config_path = path
|
self.config_path = path
|
||||||
break
|
break
|
||||||
|
|
||||||
# If none found, default to the first one for error reporting
|
|
||||||
if self.config_path is None:
|
if self.config_path is None:
|
||||||
self.config_path = potential_paths[0]
|
self.config_path = potential_paths[0]
|
||||||
|
|
||||||
@@ -131,7 +127,7 @@ class OpenRouterModelRegistry:
|
|||||||
if self.use_resources:
|
if self.use_resources:
|
||||||
# Use importlib.resources for packaged environments
|
# Use importlib.resources for packaged environments
|
||||||
try:
|
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"):
|
if hasattr(resource_path, "read_text"):
|
||||||
# Python 3.9+
|
# Python 3.9+
|
||||||
config_text = resource_path.read_text(encoding="utf-8")
|
config_text = resource_path.read_text(encoding="utf-8")
|
||||||
|
|||||||
@@ -12,7 +12,7 @@ dependencies = [
|
|||||||
]
|
]
|
||||||
|
|
||||||
[tool.setuptools.packages.find]
|
[tool.setuptools.packages.find]
|
||||||
include = ["tools*", "providers*", "systemprompts*", "utils*"]
|
include = ["tools*", "providers*", "systemprompts*", "utils*", "conf*"]
|
||||||
|
|
||||||
[tool.setuptools]
|
[tool.setuptools]
|
||||||
py-modules = ["server", "config"]
|
py-modules = ["server", "config"]
|
||||||
|
|||||||
@@ -53,7 +53,7 @@ class TestUvxPathResolution:
|
|||||||
assert registry.config_path == config_path
|
assert registry.config_path == config_path
|
||||||
assert len(registry.list_models()) > 0
|
assert len(registry.list_models()) > 0
|
||||||
|
|
||||||
@patch("providers.openrouter_registry.files")
|
@patch("providers.openrouter_registry.importlib.resources.files")
|
||||||
@patch("pathlib.Path.exists")
|
@patch("pathlib.Path.exists")
|
||||||
def test_multiple_path_fallback(self, mock_exists, mock_files):
|
def test_multiple_path_fallback(self, mock_exists, mock_files):
|
||||||
"""Test that multiple path resolution works for different deployment scenarios."""
|
"""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 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 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
|
# Should load models successfully
|
||||||
assert len(registry.list_models()) > 0
|
assert len(registry.list_models()) > 0
|
||||||
|
|||||||
Reference in New Issue
Block a user