From 60adff1a5af6584a3bac7e8e994b1dd352b26217 Mon Sep 17 00:00:00 2001 From: Tim Monko Date: Tue, 2 Dec 2025 11:23:48 -0600 Subject: [PATCH 01/13] use caching for settings file to improve load time --- pyproject.toml | 5 +- src/ndev_settings/__init__.py | 4 +- src/ndev_settings/_settings.py | 331 ++++++++++++++++---------- src/ndev_settings/_settings_widget.py | 2 +- tests/conftest.py | 26 +- tests/test_settings.py | 59 +++-- tox.ini | 2 +- 7 files changed, 263 insertions(+), 166 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index fde03cb..3e3ac86 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -37,15 +37,16 @@ dependencies = [ [project.optional-dependencies] # Allow easily installation with the full, default napari installation # (including Qt backend) using ndev-settings[all]. -all = ["napari[all]"] +all = ["napari[pyqt6]"] [dependency-groups] -testing = [ +dev = [ "tox-uv", "pytest", # https://docs.pytest.org/en/latest/contents.html "pytest-cov", # https://pytest-cov.readthedocs.io/en/latest/ "pytest-qt", # https://pytest-qt.readthedocs.io/en/latest/ "pyqt6", + "napari", ] [project.entry-points."napari.manifest"] diff --git a/src/ndev_settings/__init__.py b/src/ndev_settings/__init__.py index 30761d6..eea74d0 100644 --- a/src/ndev_settings/__init__.py +++ b/src/ndev_settings/__init__.py @@ -1,7 +1,7 @@ -from ._settings import Settings +from ._settings import Settings, clear_settings from ._version import version as __version__ -__all__ = ["get_settings", "__version__"] +__all__ = ["get_settings", "clear_settings", "__version__"] # Singleton instance _settings_instance = None diff --git a/src/ndev_settings/_settings.py b/src/ndev_settings/_settings.py index a1bcbb3..1c976c0 100644 --- a/src/ndev_settings/_settings.py +++ b/src/ndev_settings/_settings.py @@ -1,160 +1,231 @@ +from __future__ import annotations + +import hashlib +import os from importlib.metadata import entry_points +from pathlib import Path import yaml +# User settings are stored in a single cache file +_SETTINGS_DIR = ( + Path(os.environ.get("XDG_CONFIG_HOME", Path.home() / ".config")) + / "ndev-settings" +) +_SETTINGS_FILE = _SETTINGS_DIR / "settings.yaml" + +# Module-level flag to disable persistence (used in tests) +_persistence_enabled = True + + +def _load_yaml(path: Path) -> dict: + """Load a YAML file, returning empty dict if missing or invalid.""" + try: + with open(path) as f: + return yaml.load(f, Loader=yaml.FullLoader) or {} + except (FileNotFoundError, yaml.YAMLError, OSError): + return {} + + +def _get_entry_points_hash() -> str: + """Generate a hash of installed ndev_settings.manifest entry points. + + Used to detect when packages are installed/removed. + """ + eps = entry_points(group="ndev_settings.manifest") + ep_strings = sorted(f"{ep.name}:{ep.value}" for ep in eps) + return hashlib.md5("|".join(ep_strings).encode()).hexdigest() + + +def clear_settings() -> None: + """Clear saved settings. Next load will use fresh defaults.""" + try: + if _SETTINGS_FILE.exists(): + _SETTINGS_FILE.unlink() + except OSError: + pass + class SettingsGroup: """Simple container for settings in a group.""" class Settings: - """A class to manage settings for the nDev plugin, with nested group objects.""" + """Manages persistent settings for ndev plugins. + + Settings are discovered from YAML files in installed packages (via entry points), + merged together, and stored in a single user settings file. User modifications + persist across sessions. + + When packages are installed/removed, new settings are merged in while + preserving existing user values. + """ + + def __init__(self, defaults_file: str | None = None): + """Initialize settings. + + Parameters + ---------- + defaults_file : str, optional + Path to a YAML file with default settings. Usually the main + ndev_settings.yaml file. External contributions are merged in. + """ + self._defaults_path = Path(defaults_file) if defaults_file else None + self._grouped_settings: dict = {} + self._load() + + def _load(self): + """Load settings from saved file or discover from package YAML files. + + On first load (or after package changes), discovers settings from all + installed packages and saves them. Subsequent loads just read the saved file. + """ + saved = self._load_saved() + + if saved is not None: + # Fast path: use saved settings + all_settings = saved + else: + # Slow path: discover from package YAML files, then save + all_settings = self._load_defaults() + self._save_settings(all_settings) + + # Build group objects and store + self._build_groups(all_settings) + self._grouped_settings = all_settings + + def _load_defaults(self) -> dict: + """Load default settings from main file and external contributions.""" + all_settings = {} + + # Load main defaults file if provided + if self._defaults_path: + all_settings = _load_yaml(self._defaults_path) + + # Load external YAML files from entry points + for ep in entry_points(group="ndev_settings.manifest"): + try: + package_name, resource_name = ep.value.split(":", 1) + from importlib.resources import files + + yaml_path = Path(str(files(package_name) / resource_name)) + external = _load_yaml(yaml_path) + + # Merge external settings (first one wins for conflicts) + for group_name, group_settings in external.items(): + if group_name not in all_settings: + all_settings[group_name] = {} + for name, data in group_settings.items(): + if name not in all_settings[group_name]: + all_settings[group_name][name] = data + except (ModuleNotFoundError, FileNotFoundError, ValueError) as e: + print(f"Failed to load settings from '{ep.name}': {e}") + + return all_settings + + def _load_saved(self) -> dict | None: + """Load saved settings if valid, return None if stale or missing.""" + if not _persistence_enabled or not _SETTINGS_FILE.exists(): + return None + + saved = _load_yaml(_SETTINGS_FILE) + if not saved: + return None + + # Check if packages changed - if so, need to re-discover + saved_hash = saved.get("_entry_points_hash") + if saved_hash != _get_entry_points_hash(): + # Packages installed/removed - merge new defaults with saved values + defaults = self._load_defaults() + saved_settings = saved.get("settings", {}) + merged = self._merge_with_saved(defaults, saved_settings) + return merged + + return saved.get("settings") + + def _merge_with_saved(self, defaults: dict, saved: dict) -> dict: + """Merge saved user values into fresh defaults.""" + merged = {} + for group_name, group_settings in defaults.items(): + merged[group_name] = {} + for name, data in group_settings.items(): + merged[group_name][name] = data.copy() + # Override with saved value if exists + if group_name in saved and name in saved[group_name]: + saved_data = saved[group_name][name] + if isinstance(saved_data, dict) and "value" in saved_data: + merged[group_name][name]["value"] = saved_data["value"] + return merged + + def _save_settings(self, settings: dict) -> None: + """Save settings to file.""" + if not _persistence_enabled: + return + try: + _SETTINGS_DIR.mkdir(parents=True, exist_ok=True) + data = { + "_entry_points_hash": _get_entry_points_hash(), + "settings": settings, + } + with open(_SETTINGS_FILE, "w") as f: + yaml.dump(data, f, default_flow_style=False, sort_keys=False) + except OSError: + pass + + def _build_groups(self, settings: dict): + """Create SettingsGroup objects from settings dict.""" + for group_name, group_settings in settings.items(): + group_obj = SettingsGroup() + for name, setting_data in group_settings.items(): + if isinstance(setting_data, dict) and "value" in setting_data: + setattr(group_obj, name, setting_data["value"]) + setattr(self, group_name, group_obj) - def __init__(self, settings_file: str): - self._settings_path = settings_file - self.load() + def _sync_groups_to_dict(self): + """Sync current group object values back to _grouped_settings dict.""" + for group_name, group_settings in self._grouped_settings.items(): + group_obj = getattr(self, group_name, None) + if isinstance(group_obj, SettingsGroup): + for setting_name in group_settings: + if hasattr(group_obj, setting_name): + value = getattr(group_obj, setting_name) + self._grouped_settings[group_name][setting_name][ + "value" + ] = value + + def save(self): + """Save current settings to persist across sessions.""" + self._sync_groups_to_dict() + self._save_settings(self._grouped_settings) def reset_to_default( self, setting_name: str | None = None, group: str | None = None ): """Reset a setting (or all settings) to their default values.""" if setting_name: - # Reset single setting - find it in any group + # Reset single setting for group_name, group_settings in self._grouped_settings.items(): if setting_name in group_settings: - setting_data = group_settings[setting_name] - default_value = setting_data["default"] - - # Update both the SettingsGroup object and _grouped_settings - setattr( - getattr(self, group_name), setting_name, default_value - ) - setting_data["value"] = default_value - + default = group_settings[setting_name].get("default") + if default is not None: + setattr( + getattr(self, group_name), setting_name, default + ) + group_settings[setting_name]["value"] = default self.save() return else: - # Reset all settings (optionally by group) + # Reset all settings (optionally filtered by group) for group_name, group_settings in self._grouped_settings.items(): if group and group_name != group: continue for name, setting_data in group_settings.items(): if "default" in setting_data: - default_value = setting_data["default"] - - # Update both the SettingsGroup object and _grouped_settings - setattr(getattr(self, group_name), name, default_value) - setting_data["value"] = default_value + default = setting_data["default"] + setattr(getattr(self, group_name), name, default) + setting_data["value"] = default self.save() - def _get_dynamic_choices(self, provider_key: str) -> list: + def get_dynamic_choices(self, provider_key: str) -> list: """Get dynamic choices from entry points.""" - entries = entry_points(group=provider_key) - return [entry.name for entry in entries] - - def load(self): - """Load settings from main file, external YAML files, and entry points.""" - # Start with main settings file - all_settings = self._load_yaml_file(self._settings_path) - - # Load external YAML files from entry points - external_yaml_settings = self._load_external_yaml_files() - - # Merge external YAML settings - append new groups at the end to preserve order - # First, add settings to existing groups - for group_name, group_settings in external_yaml_settings.items(): - if group_name in all_settings: - # Add to existing group (main file settings take precedence) - for setting_name, setting_data in group_settings.items(): - if setting_name not in all_settings[group_name]: - all_settings[group_name][setting_name] = setting_data - - # Then, add completely new groups at the end - for group_name, group_settings in external_yaml_settings.items(): - if group_name not in all_settings: - all_settings[group_name] = group_settings - - # Create group objects from merged settings - for group_name, group_settings in all_settings.items(): - group_obj = SettingsGroup() - for name, setting_data in group_settings.items(): - if isinstance(setting_data, dict) and "value" in setting_data: - value = setting_data["value"] - setattr(group_obj, name, value) - setattr(self, group_name, group_obj) - - self._grouped_settings = all_settings - # Don't auto-save during load - only save when explicitly requested - # This prevents modifying files during testing and keeps loading fast - - def _load_yaml_file(self, yaml_path: str) -> dict: - """Load a single YAML settings file.""" - try: - with open(yaml_path) as file: - # Use FullLoader to support Python-specific tags like !!python/tuple - return yaml.load(file, Loader=yaml.FullLoader) or {} - except FileNotFoundError: - return {} - - def _load_external_yaml_files(self) -> dict: - """Load external YAML files from other packages via entry points.""" - all_external_settings = {} - # Look for entry points that provide YAML file paths - for entry_point in entry_points(group="ndev_settings.manifest"): - try: - # Support napari-style resource paths (e.g., "package:file.yaml") - entry_value = entry_point.value - - package_name, resource_name = entry_value.split(":", 1) - - from importlib.resources import files - - yaml_path = str(files(package_name) / resource_name) - external_settings = self._load_yaml_file(yaml_path) - # Merge with all external settings - for ( - group_name, - group_settings, - ) in external_settings.items(): - if group_name not in all_external_settings: - all_external_settings[group_name] = {} - all_external_settings[group_name].update(group_settings) - except ( - ModuleNotFoundError, - FileNotFoundError, - AttributeError, - ValueError, - ) as e: - print( - f"Failed to load external settings from entry point '{entry_point.name}': {e}" - ) - - return all_external_settings - - def save(self): - """Save the current state of all settings to the YAML file, preserving original order.""" - # Update the _grouped_settings values from the current group objects - for group_name, group_settings in self._grouped_settings.items(): - if hasattr(self, group_name): - group_obj = getattr(self, group_name) - if isinstance(group_obj, SettingsGroup): - for setting_name in group_settings: - if hasattr(group_obj, setting_name): - current_value = getattr(group_obj, setting_name) - # Update the value in place - self._grouped_settings[group_name][setting_name][ - "value" - ] = current_value - - # Save the updated _grouped_settings directly - self._save_settings_file(self._grouped_settings) - - def _save_settings_file(self, settings_data): - """Helper to save settings data to file.""" - with open(self._settings_path, "w") as file: - yaml.dump( - settings_data, - file, - default_flow_style=False, - sort_keys=False, - ) + return [ep.name for ep in entry_points(group=provider_key)] diff --git a/src/ndev_settings/_settings_widget.py b/src/ndev_settings/_settings_widget.py index 322a5d4..8fa5a4a 100644 --- a/src/ndev_settings/_settings_widget.py +++ b/src/ndev_settings/_settings_widget.py @@ -21,7 +21,7 @@ def _get_dynamic_choices(self, setting_info: dict) -> tuple[list, str]: "fallback_message", "No choices available" ) - choices = self.settings._get_dynamic_choices(provider) + choices = self.settings.get_dynamic_choices(provider) return choices if choices else [fallback_message], fallback_message def _create_widget_for_setting( diff --git a/tests/conftest.py b/tests/conftest.py index e1207a4..3e53eaa 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -6,6 +6,18 @@ import pytest +@pytest.fixture(autouse=True) +def disable_settings_persistence(monkeypatch): + """Disable settings persistence during tests to ensure clean state.""" + from ndev_settings import _settings + + # Clear any existing saved settings + _settings.clear_settings() + + # Disable persistence via module-level flag + monkeypatch.setattr(_settings, "_persistence_enabled", False) + + @pytest.fixture(autouse=True) def mock_settings_file_path(tmp_path, monkeypatch, test_data_dir): """Mock the settings file path to use test data during tests.""" @@ -26,17 +38,17 @@ def mock_settings_file_path(tmp_path, monkeypatch, test_data_dir): Path(ndev_settings.__file__).parent / "ndev_settings.yaml" ) - def mock_settings_init(self, settings_file=None): + def mock_settings_init(self, defaults_file=None): """Mock Settings constructor to use test file only when loading the default settings.""" # Only redirect if this is the default ndev_settings.yaml path - if settings_file == real_settings_path: - settings_file = str(test_settings_path) + if defaults_file == real_settings_path: + defaults_file = str(test_settings_path) - # Ensure settings_file is never None - if settings_file is None: - settings_file = str(test_settings_path) + # Ensure defaults_file is never None + if defaults_file is None: + defaults_file = str(test_settings_path) - return original_settings_init(self, settings_file) + return original_settings_init(self, defaults_file) # Mock the Settings class constructor monkeypatch.setattr(Settings, "__init__", mock_settings_init) diff --git a/tests/test_settings.py b/tests/test_settings.py index e22d339..fcfb9e7 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -99,23 +99,32 @@ def test_reset_by_group(self, test_settings_file): class TestSettingsSaveLoad: """Test Settings save and load functionality.""" - def test_save_and_reload(self, test_settings_file): - """Test saving changes and reloading them.""" + def test_save_syncs_values(self, test_settings_file): + """Test that save() syncs group values to internal dict.""" settings = Settings(str(test_settings_file)) - # Modify some settings + # Modify some settings via group objects settings.Group_A.setting_int = 555 settings.Group_A.setting_string = "Saved text" - # Save - settings.save() + # Before save, _grouped_settings should still have old values + assert ( + settings._grouped_settings["Group_A"]["setting_int"]["value"] + != 555 + ) - # Create new Settings instance from same file - settings2 = Settings(str(test_settings_file)) + # Save syncs values + settings.save() - # Should have saved changes - assert settings2.Group_A.setting_int == 555 - assert settings2.Group_A.setting_string == "Saved text" + # Now internal dict should be updated + assert ( + settings._grouped_settings["Group_A"]["setting_int"]["value"] + == 555 + ) + assert ( + settings._grouped_settings["Group_A"]["setting_string"]["value"] + == "Saved text" + ) class TestDynamicChoices: @@ -126,13 +135,13 @@ def test_dynamic_choices_handling(self, test_settings_file): settings = Settings(str(test_settings_file)) # Test the dynamic choices method - choices = settings._get_dynamic_choices("bioio.readers") + choices = settings.get_dynamic_choices("bioio.readers") # Should return a list (even if empty) assert isinstance(choices, list) # Test with invalid provider - empty_choices = settings._get_dynamic_choices("invalid.provider") + empty_choices = settings.get_dynamic_choices("invalid.provider") assert empty_choices == [] @@ -188,18 +197,22 @@ def test_external_adds_new_setting_to_existing_group( def test_external_settings_can_be_modified( self, test_settings_file, mock_external_contributions ): - """Test that external settings can be modified and saved.""" + """Test that external settings can be modified via group objects.""" settings = Settings(str(test_settings_file)) - # Modify external setting + # Modify external setting via group object settings.External_Contribution.setting_int = 999 - # Save and reload + # Save syncs the value to internal dict settings.save() - settings2 = Settings(str(test_settings_file)) - # Should preserve the change - assert settings2.External_Contribution.setting_int == 999 + # Verify the internal dict was updated + assert ( + settings._grouped_settings["External_Contribution"]["setting_int"][ + "value" + ] + == 999 + ) def test_duplicate_external_contributions_handling( self, test_settings_file, tmp_path, monkeypatch @@ -302,11 +315,11 @@ def __truediv__(self, resource_name): # Should have the shared group assert hasattr(settings, "Shared_Group") - # Last external contribution should win (external2) - # because _load_external_yaml_files uses dict.update() which overwrites existing keys - assert settings.Shared_Group.shared_setting == "from_external2" + # First external contribution wins for overlapping settings (stable, predictable) + # This prevents unpredictable behavior based on package load order + assert settings.Shared_Group.shared_setting == "from_external1" - # But unique settings from later externals should still be added + # Unique settings from later externals should still be added assert settings.Shared_Group.unique_setting == "unique_to_external2" @@ -369,6 +382,6 @@ def test_dynamic_choices(empty_settings_file): ) # Create without calling __init__ # Test the method exists and handles missing entry points gracefully - choices = settings._get_dynamic_choices("nonexistent.entry.point") + choices = settings.get_dynamic_choices("nonexistent.entry.point") assert isinstance(choices, list) assert len(choices) == 0 # Should return empty list when no entries found diff --git a/tox.ini b/tox.ini index 1f60aaa..b736b0c 100644 --- a/tox.ini +++ b/tox.ini @@ -29,5 +29,5 @@ passenv = NUMPY_EXPERIMENTAL_ARRAY_FUNCTION PYVISTA_OFF_SCREEN dependency_groups = - testing + dev commands = pytest -v --color=yes --cov=ndev_settings --cov-report=xml From 16161bb251d56992883954185556925c6baa4131 Mon Sep 17 00:00:00 2001 From: Tim Monko Date: Tue, 2 Dec 2025 11:48:45 -0600 Subject: [PATCH 02/13] use logging, proper cache order, and update tests --- pyproject.toml | 1 + src/ndev_settings/_settings.py | 48 ++++++++----------- tests/conftest.py | 59 ++++++++++------------- tests/test_settings.py | 85 ++++++++++++++++++++++++++++++++++ 4 files changed, 131 insertions(+), 62 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 3e3ac86..b0d1d65 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -29,6 +29,7 @@ requires-python = ">=3.10" # or any other Qt bindings directly (e.g. PyQt5, PySide2). # See best practices: https://napari.org/stable/plugins/building_a_plugin/best_practices.html dependencies = [ + "appdirs", "magicgui", "magic-class", "pyyaml", diff --git a/src/ndev_settings/_settings.py b/src/ndev_settings/_settings.py index 1c976c0..a0f2b16 100644 --- a/src/ndev_settings/_settings.py +++ b/src/ndev_settings/_settings.py @@ -1,21 +1,19 @@ from __future__ import annotations import hashlib -import os +import logging from importlib.metadata import entry_points from pathlib import Path +import appdirs import yaml -# User settings are stored in a single cache file -_SETTINGS_DIR = ( - Path(os.environ.get("XDG_CONFIG_HOME", Path.home() / ".config")) - / "ndev-settings" -) -_SETTINGS_FILE = _SETTINGS_DIR / "settings.yaml" +logger = logging.getLogger(__name__) -# Module-level flag to disable persistence (used in tests) -_persistence_enabled = True +# User settings stored in platform-appropriate config directory +# Uses same location style as napari for familiarity +_SETTINGS_DIR = Path(appdirs.user_config_dir("ndev-settings", appauthor=False)) +_SETTINGS_FILE = _SETTINGS_DIR / "settings.yaml" def _load_yaml(path: Path) -> dict: @@ -39,11 +37,8 @@ def _get_entry_points_hash() -> str: def clear_settings() -> None: """Clear saved settings. Next load will use fresh defaults.""" - try: - if _SETTINGS_FILE.exists(): - _SETTINGS_FILE.unlink() - except OSError: - pass + if _SETTINGS_FILE.exists(): + _SETTINGS_FILE.unlink() class SettingsGroup: @@ -119,13 +114,15 @@ def _load_defaults(self) -> dict: if name not in all_settings[group_name]: all_settings[group_name][name] = data except (ModuleNotFoundError, FileNotFoundError, ValueError) as e: - print(f"Failed to load settings from '{ep.name}': {e}") + logger.warning( + "Failed to load settings from '%s': %s", ep.name, e + ) return all_settings def _load_saved(self) -> dict | None: """Load saved settings if valid, return None if stale or missing.""" - if not _persistence_enabled or not _SETTINGS_FILE.exists(): + if not _SETTINGS_FILE.exists(): return None saved = _load_yaml(_SETTINGS_FILE) @@ -159,18 +156,13 @@ def _merge_with_saved(self, defaults: dict, saved: dict) -> dict: def _save_settings(self, settings: dict) -> None: """Save settings to file.""" - if not _persistence_enabled: - return - try: - _SETTINGS_DIR.mkdir(parents=True, exist_ok=True) - data = { - "_entry_points_hash": _get_entry_points_hash(), - "settings": settings, - } - with open(_SETTINGS_FILE, "w") as f: - yaml.dump(data, f, default_flow_style=False, sort_keys=False) - except OSError: - pass + _SETTINGS_DIR.mkdir(parents=True, exist_ok=True) + data = { + "_entry_points_hash": _get_entry_points_hash(), + "settings": settings, + } + with open(_SETTINGS_FILE, "w") as f: + yaml.dump(data, f, default_flow_style=False, sort_keys=False) def _build_groups(self, settings: dict): """Create SettingsGroup objects from settings dict.""" diff --git a/tests/conftest.py b/tests/conftest.py index 3e53eaa..0e0919c 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -7,58 +7,49 @@ @pytest.fixture(autouse=True) -def disable_settings_persistence(monkeypatch): - """Disable settings persistence during tests to ensure clean state.""" - from ndev_settings import _settings - - # Clear any existing saved settings - _settings.clear_settings() - - # Disable persistence via module-level flag - monkeypatch.setattr(_settings, "_persistence_enabled", False) +def isolate_settings(tmp_path, monkeypatch, test_data_dir): + """Isolate settings to tmp_path for each test. + This redirects both: + 1. The settings file location (where user settings are saved) + 2. The defaults file (to use test data instead of real package defaults) -@pytest.fixture(autouse=True) -def mock_settings_file_path(tmp_path, monkeypatch, test_data_dir): - """Mock the settings file path to use test data during tests.""" - from pathlib import Path - + This allows tests to run with real persistence behavior while being isolated. + """ import ndev_settings + from ndev_settings import _settings from ndev_settings._settings import Settings - # Use the existing test_settings.yaml as our mock data - test_settings_path = tmp_path / "test_ndev_settings.yaml" - shutil.copy(test_data_dir / "test_settings.yaml", test_settings_path) + # Redirect settings file to tmp_path + test_settings_dir = tmp_path / "ndev-settings" + test_settings_file = test_settings_dir / "settings.yaml" + monkeypatch.setattr(_settings, "_SETTINGS_DIR", test_settings_dir) + monkeypatch.setattr(_settings, "_SETTINGS_FILE", test_settings_file) - # Store the original Settings class constructor - original_settings_init = Settings.__init__ + # Copy test data to use as defaults + test_defaults_path = tmp_path / "test_defaults.yaml" + shutil.copy(test_data_dir / "test_settings.yaml", test_defaults_path) - # Get the real default settings file path + # Redirect default settings file path + original_settings_init = Settings.__init__ real_settings_path = str( Path(ndev_settings.__file__).parent / "ndev_settings.yaml" ) def mock_settings_init(self, defaults_file=None): - """Mock Settings constructor to use test file only when loading the default settings.""" - # Only redirect if this is the default ndev_settings.yaml path - if defaults_file == real_settings_path: - defaults_file = str(test_settings_path) - - # Ensure defaults_file is never None - if defaults_file is None: - defaults_file = str(test_settings_path) - + """Redirect default settings file to test data.""" + if defaults_file == real_settings_path or defaults_file is None: + defaults_file = str(test_defaults_path) return original_settings_init(self, defaults_file) - # Mock the Settings class constructor monkeypatch.setattr(Settings, "__init__", mock_settings_init) - # Reset singleton before test + # Reset singleton before each test ndev_settings._settings_instance = None - yield test_settings_path + yield test_defaults_path - # Clean up + # Clean up singleton after test ndev_settings._settings_instance = None @@ -94,7 +85,7 @@ def empty_settings_file(tmp_path): @pytest.fixture def mock_external_contributions(tmp_path, test_data_dir, monkeypatch): - """Mock entry points that provide external YAML contributions using napari-style resource paths.""" + """Mock entry points that provide external YAML contributions.""" # Copy external contribution file to temp directory external_file = tmp_path / "external_contribution.yaml" diff --git a/tests/test_settings.py b/tests/test_settings.py index fcfb9e7..2a2fe6f 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -126,6 +126,91 @@ def test_save_syncs_values(self, test_settings_file): == "Saved text" ) + def test_save_persists_across_instances(self, test_settings_file): + """Test that saved settings are loaded by new instances.""" + settings1 = Settings(str(test_settings_file)) + + # Modify and save + settings1.Group_A.setting_int = 999 + settings1.save() + + # Create new instance - should load saved value + settings2 = Settings(str(test_settings_file)) + assert settings2.Group_A.setting_int == 999 + + def test_cached_load_is_faster(self, test_settings_file): + """Test that second load uses cached settings.""" + import time + + # First load - discovers from files + start = time.perf_counter() + settings1 = Settings(str(test_settings_file)) + first_load_time = time.perf_counter() - start + + # Second load - should use cached file + start = time.perf_counter() + settings2 = Settings(str(test_settings_file)) + second_load_time = time.perf_counter() - start + + # Both should have same values + assert settings1.Group_A.setting_int == settings2.Group_A.setting_int + + # Second load should be faster (or at least not slower) + # Note: On fast systems both may be very fast, so we just check it works + assert second_load_time <= first_load_time + assert settings2._grouped_settings is not None + + +class TestCachingBehavior: + """Test settings caching and persistence behavior.""" + + def test_clear_settings_forces_rediscovery(self, test_settings_file): + """Test that clear_settings() forces re-discovery from defaults.""" + from ndev_settings import _settings + + settings1 = Settings(str(test_settings_file)) + settings1.Group_A.setting_int = 888 + settings1.save() + + # Clear settings + _settings.clear_settings() + + # New instance should have default values + settings2 = Settings(str(test_settings_file)) + assert ( + settings2.Group_A.setting_int == 49 + ) # default from test_settings.yaml + + def test_package_change_preserves_user_values( + self, test_settings_file, monkeypatch + ): + """Test that when packages change, user values are preserved.""" + from ndev_settings import _settings + + # First load and save custom values + settings1 = Settings(str(test_settings_file)) + settings1.Group_A.setting_int = 777 + settings1.save() + + # Simulate a package change by modifying the hash function + _ = _settings._get_entry_points_hash() + monkeypatch.setattr( + _settings, "_get_entry_points_hash", lambda: "different_hash" + ) + + # New instance should merge: new defaults + saved user values + settings2 = Settings(str(test_settings_file)) + + # User's custom value should be preserved + assert settings2.Group_A.setting_int == 777 + + def test_clear_settings_handles_missing_file(self): + """Test that clear_settings doesn't crash if file doesn't exist.""" + from ndev_settings import _settings + + # This should not raise even if file doesn't exist + _settings.clear_settings() + class TestDynamicChoices: """Test dynamic choices functionality.""" From 519eee575a8462bc538ed224b34eaebc56a6931d Mon Sep 17 00:00:00 2001 From: Tim Monko Date: Tue, 2 Dec 2025 11:53:34 -0600 Subject: [PATCH 03/13] update README and add tests for cli.py --- README.md | 30 ++++++ tests/test_cli.py | 247 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 277 insertions(+) create mode 100644 tests/test_cli.py diff --git a/README.md b/README.md index 4381972..ef0f742 100644 --- a/README.md +++ b/README.md @@ -117,6 +117,36 @@ print(settings.Canvas.canvas_scale) # Access settings from external libraries (if installed) print(settings.Reader.preferred_reader) # From ndevio print(settings.Export.compression_level) # From ndevio + +# Modify and save settings +settings.Canvas.canvas_scale = 2.0 +settings.save() # Persists across sessions + +# Reset to defaults +settings.reset_to_default("canvas_scale") # Reset single setting +settings.reset_to_default(group="Canvas") # Reset entire group +settings.reset_to_default() # Reset all settings +``` + +## How Settings Persistence Works + +Settings are automatically cached to improve startup performance: + +1. **First load**: Settings are discovered from all installed packages via entry points, merged together, and saved to a user config file +2. **Subsequent loads**: Settings are loaded directly from the cached file (much faster) +3. **Package changes**: When packages are installed/removed, settings are re-discovered and merged while preserving your customizations + +**Storage location**: Settings are stored in a platform-appropriate config directory: + +- **Windows**: `%LOCALAPPDATA%\ndev-settings\settings.yaml` +- **macOS**: `~/Library/Application Support/ndev-settings/settings.yaml` +- **Linux**: `~/.config/ndev-settings/settings.yaml` + +**Clearing the cache**: To force re-discovery of settings (e.g., after manual edits to package YAML files): + +```python +from ndev_settings._settings import clear_settings +clear_settings() # Deletes cached settings, next load will re-discover ``` ## Pre-commit hook diff --git a/tests/test_cli.py b/tests/test_cli.py new file mode 100644 index 0000000..f2d1e97 --- /dev/null +++ b/tests/test_cli.py @@ -0,0 +1,247 @@ +"""Tests for ndev-settings CLI utilities.""" + +import sys + +import yaml + +from ndev_settings._cli import main_reset_values, reset_values_to_defaults + + +class TestResetValuesToDefaults: + """Tests for reset_values_to_defaults function.""" + + def test_reset_modified_values(self, tmp_path): + """Test that modified values are reset to defaults.""" + settings_file = tmp_path / "settings.yaml" + settings_file.write_text( + yaml.dump( + { + "TestGroup": { + "setting1": { + "value": "modified", + "default": "original", + }, + "setting2": {"value": 100, "default": 50}, + } + } + ) + ) + + result = reset_values_to_defaults(settings_file) + + assert result is True + with open(settings_file) as f: + updated = yaml.safe_load(f) + assert updated["TestGroup"]["setting1"]["value"] == "original" + assert updated["TestGroup"]["setting2"]["value"] == 50 + + def test_no_changes_when_values_match_defaults(self, tmp_path): + """Test that no changes are made when values already match defaults.""" + settings_file = tmp_path / "settings.yaml" + settings_file.write_text( + yaml.dump( + { + "TestGroup": { + "setting1": { + "value": "original", + "default": "original", + }, + } + } + ) + ) + + result = reset_values_to_defaults(settings_file) + + assert result is False + + def test_missing_file_returns_false(self, tmp_path, capsys): + """Test that missing file returns False and prints message.""" + missing_file = tmp_path / "nonexistent.yaml" + + result = reset_values_to_defaults(missing_file) + + assert result is False + captured = capsys.readouterr() + assert "Settings file not found" in captured.out + + def test_empty_file_returns_false(self, tmp_path): + """Test that empty file returns False.""" + settings_file = tmp_path / "empty.yaml" + settings_file.write_text("") + + result = reset_values_to_defaults(settings_file) + + assert result is False + + def test_settings_without_default_key_unchanged(self, tmp_path): + """Test that settings without 'default' key are not modified.""" + settings_file = tmp_path / "settings.yaml" + settings_file.write_text( + yaml.dump( + { + "TestGroup": { + "setting_no_default": {"value": "something"}, + } + } + ) + ) + + result = reset_values_to_defaults(settings_file) + + assert result is False + + def test_settings_without_value_key_unchanged(self, tmp_path): + """Test that settings without 'value' key are not modified.""" + settings_file = tmp_path / "settings.yaml" + settings_file.write_text( + yaml.dump( + { + "TestGroup": { + "setting_no_value": {"default": "something"}, + } + } + ) + ) + + result = reset_values_to_defaults(settings_file) + + assert result is False + + def test_accepts_string_path(self, tmp_path): + """Test that string paths are accepted.""" + settings_file = tmp_path / "settings.yaml" + settings_file.write_text( + yaml.dump( + { + "TestGroup": { + "setting1": { + "value": "modified", + "default": "original", + }, + } + } + ) + ) + + result = reset_values_to_defaults(str(settings_file)) + + assert result is True + + +class TestMainResetValues: + """Tests for main_reset_values CLI entry point.""" + + def test_no_args_shows_usage(self, monkeypatch, capsys): + """Test that no arguments shows usage and returns 1.""" + monkeypatch.setattr(sys, "argv", ["reset-settings-values"]) + + result = main_reset_values() + + assert result == 1 + captured = capsys.readouterr() + assert "Usage:" in captured.out + assert "pre-commit hook" in captured.out + + def test_single_file_modified(self, tmp_path, monkeypatch, capsys): + """Test processing a single file that needs modification.""" + settings_file = tmp_path / "settings.yaml" + settings_file.write_text( + yaml.dump( + { + "TestGroup": { + "setting1": { + "value": "modified", + "default": "original", + }, + } + } + ) + ) + monkeypatch.setattr( + sys, "argv", ["reset-settings-values", str(settings_file)] + ) + + result = main_reset_values() + + assert result == 1 + captured = capsys.readouterr() + assert "WARNING" in captured.out + assert "re-stage" in captured.out + + def test_single_file_unchanged(self, tmp_path, monkeypatch): + """Test processing a single file that doesn't need modification.""" + settings_file = tmp_path / "settings.yaml" + settings_file.write_text( + yaml.dump( + { + "TestGroup": { + "setting1": { + "value": "original", + "default": "original", + }, + } + } + ) + ) + monkeypatch.setattr( + sys, "argv", ["reset-settings-values", str(settings_file)] + ) + + result = main_reset_values() + + assert result == 0 + + def test_multiple_files(self, tmp_path, monkeypatch, capsys): + """Test processing multiple files.""" + file1 = tmp_path / "settings1.yaml" + file2 = tmp_path / "settings2.yaml" + file1.write_text( + yaml.dump( + { + "Group1": { + "s1": {"value": "modified", "default": "original"} + } + } + ) + ) + file2.write_text( + yaml.dump( + { + "Group2": { + "s2": {"value": "original", "default": "original"} + } + } + ) + ) + monkeypatch.setattr( + sys, "argv", ["reset-settings-values", str(file1), str(file2)] + ) + + result = main_reset_values() + + assert result == 1 # At least one file was modified + captured = capsys.readouterr() + assert "Resetting Group1.s1" in captured.out + + def test_missing_file_in_list(self, tmp_path, monkeypatch, capsys): + """Test that missing files are handled gracefully.""" + existing_file = tmp_path / "exists.yaml" + existing_file.write_text( + yaml.dump( + {"Group": {"s": {"value": "original", "default": "original"}}} + ) + ) + missing_file = tmp_path / "missing.yaml" + + monkeypatch.setattr( + sys, + "argv", + ["reset-settings-values", str(existing_file), str(missing_file)], + ) + + result = main_reset_values() + + assert result == 0 # No modifications made + captured = capsys.readouterr() + assert "not found" in captured.out From a12158b6344aac11c84b2dac2dbc1a8d1a19428c Mon Sep 17 00:00:00 2001 From: Tim Monko Date: Tue, 2 Dec 2025 12:06:01 -0600 Subject: [PATCH 04/13] Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- README.md | 2 +- src/ndev_settings/_settings.py | 19 +++++++++++-------- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index ef0f742..66b464b 100644 --- a/README.md +++ b/README.md @@ -145,7 +145,7 @@ Settings are automatically cached to improve startup performance: **Clearing the cache**: To force re-discovery of settings (e.g., after manual edits to package YAML files): ```python -from ndev_settings._settings import clear_settings +from ndev_settings import clear_settings clear_settings() # Deletes cached settings, next load will re-discover ``` diff --git a/src/ndev_settings/_settings.py b/src/ndev_settings/_settings.py index a0f2b16..633aa17 100644 --- a/src/ndev_settings/_settings.py +++ b/src/ndev_settings/_settings.py @@ -136,6 +136,7 @@ def _load_saved(self) -> dict | None: defaults = self._load_defaults() saved_settings = saved.get("settings", {}) merged = self._merge_with_saved(defaults, saved_settings) + self._save_settings(merged) return merged return saved.get("settings") @@ -156,14 +157,16 @@ def _merge_with_saved(self, defaults: dict, saved: dict) -> dict: def _save_settings(self, settings: dict) -> None: """Save settings to file.""" - _SETTINGS_DIR.mkdir(parents=True, exist_ok=True) - data = { - "_entry_points_hash": _get_entry_points_hash(), - "settings": settings, - } - with open(_SETTINGS_FILE, "w") as f: - yaml.dump(data, f, default_flow_style=False, sort_keys=False) - + try: + _SETTINGS_DIR.mkdir(parents=True, exist_ok=True) + data = { + "_entry_points_hash": _get_entry_points_hash(), + "settings": settings, + } + with open(_SETTINGS_FILE, "w") as f: + yaml.dump(data, f, default_flow_style=False, sort_keys=False) + except (OSError, PermissionError) as e: + logger.warning("Failed to save settings to %s: %s", _SETTINGS_FILE, e) def _build_groups(self, settings: dict): """Create SettingsGroup objects from settings dict.""" for group_name, group_settings in settings.items(): From e2b9d607e284906f9e50a984c923d619ebcb8068 Mon Sep 17 00:00:00 2001 From: Tim Monko Date: Tue, 2 Dec 2025 12:11:30 -0600 Subject: [PATCH 05/13] ignore hash and sort entry points --- src/ndev_settings/_settings.py | 21 +++++++++++++++++--- tests/test_settings.py | 36 +++++++++++++++++++++------------- 2 files changed, 40 insertions(+), 17 deletions(-) diff --git a/src/ndev_settings/_settings.py b/src/ndev_settings/_settings.py index 633aa17..ebf5169 100644 --- a/src/ndev_settings/_settings.py +++ b/src/ndev_settings/_settings.py @@ -17,7 +17,11 @@ def _load_yaml(path: Path) -> dict: - """Load a YAML file, returning empty dict if missing or invalid.""" + """Load a YAML file, returning empty dict if missing or invalid. + + Uses FullLoader to support Python-specific types like tuples + (e.g., canvas_size: !!python/tuple [1024, 1024]) in settings files. + """ try: with open(path) as f: return yaml.load(f, Loader=yaml.FullLoader) or {} @@ -98,7 +102,12 @@ def _load_defaults(self) -> dict: all_settings = _load_yaml(self._defaults_path) # Load external YAML files from entry points - for ep in entry_points(group="ndev_settings.manifest"): + # Sort for deterministic merge order across environments + eps = sorted( + entry_points(group="ndev_settings.manifest"), + key=lambda ep: (ep.name, ep.value), + ) + for ep in eps: try: package_name, resource_name = ep.value.split(":", 1) from importlib.resources import files @@ -166,10 +175,16 @@ def _save_settings(self, settings: dict) -> None: with open(_SETTINGS_FILE, "w") as f: yaml.dump(data, f, default_flow_style=False, sort_keys=False) except (OSError, PermissionError) as e: - logger.warning("Failed to save settings to %s: %s", _SETTINGS_FILE, e) + logger.warning( + "Failed to save settings to %s: %s", _SETTINGS_FILE, e + ) + def _build_groups(self, settings: dict): """Create SettingsGroup objects from settings dict.""" for group_name, group_settings in settings.items(): + # Skip metadata keys (e.g., _entry_points_hash) + if group_name.startswith("_"): + continue group_obj = SettingsGroup() for name, setting_data in group_settings.items(): if isinstance(setting_data, dict) and "value" in setting_data: diff --git a/tests/test_settings.py b/tests/test_settings.py index 2a2fe6f..398fc4f 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -138,28 +138,36 @@ def test_save_persists_across_instances(self, test_settings_file): settings2 = Settings(str(test_settings_file)) assert settings2.Group_A.setting_int == 999 - def test_cached_load_is_faster(self, test_settings_file): - """Test that second load uses cached settings.""" - import time - - # First load - discovers from files - start = time.perf_counter() + def test_cached_load_uses_saved_file( + self, test_settings_file, monkeypatch + ): + """Test that second load uses cached settings file, not _load_defaults.""" + # First load - discovers from files and saves settings1 = Settings(str(test_settings_file)) - first_load_time = time.perf_counter() - start - # Second load - should use cached file - start = time.perf_counter() + # Track if _load_defaults is called on second load + load_defaults_called = False + original_load_defaults = Settings._load_defaults + + def tracking_load_defaults(self): + nonlocal load_defaults_called + load_defaults_called = True + return original_load_defaults(self) + + monkeypatch.setattr(Settings, "_load_defaults", tracking_load_defaults) + + # Second load - should use cached file, not call _load_defaults settings2 = Settings(str(test_settings_file)) - second_load_time = time.perf_counter() - start # Both should have same values assert settings1.Group_A.setting_int == settings2.Group_A.setting_int - - # Second load should be faster (or at least not slower) - # Note: On fast systems both may be very fast, so we just check it works - assert second_load_time <= first_load_time assert settings2._grouped_settings is not None + # _load_defaults should NOT have been called (cache was used) + assert ( + not load_defaults_called + ), "Expected cached path, but _load_defaults was called" + class TestCachingBehavior: """Test settings caching and persistence behavior.""" From f366f6ae716cb3e444d080523818aa7af1067e4b Mon Sep 17 00:00:00 2001 From: Tim Monko Date: Tue, 2 Dec 2025 12:35:01 -0600 Subject: [PATCH 06/13] use distribution path instead of import of lib --- src/ndev_settings/_settings.py | 23 ++++++++++++-- tests/conftest.py | 47 ++++++++++++++++++++------- tests/test_settings.py | 58 +++++++++++++++++++++++----------- 3 files changed, 96 insertions(+), 32 deletions(-) diff --git a/src/ndev_settings/_settings.py b/src/ndev_settings/_settings.py index ebf5169..95625d0 100644 --- a/src/ndev_settings/_settings.py +++ b/src/ndev_settings/_settings.py @@ -110,9 +110,28 @@ def _load_defaults(self) -> dict: for ep in eps: try: package_name, resource_name = ep.value.split(":", 1) - from importlib.resources import files - yaml_path = Path(str(files(package_name) / resource_name)) + # Use distribution() to find package location WITHOUT importing it + # This avoids slow package imports (e.g., ndevio takes 2.5s to import) + from importlib.metadata import distribution + + dist = distribution(package_name) + + # For installed packages, locate the resource file + # The package files are relative to the distribution location + yaml_path = None + for file in dist.files or []: + if file.name == resource_name and package_name in str( + file + ): + yaml_path = Path(str(dist.locate_file(file))) + break + + if yaml_path is None: + # Fallback: try direct path construction for editable installs + package_location = str(dist.locate_file(package_name)) + yaml_path = Path(package_location) / resource_name + external = _load_yaml(yaml_path) # Merge external settings (first one wins for conflicts) diff --git a/tests/conftest.py b/tests/conftest.py index 0e0919c..d0aca3d 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -96,6 +96,8 @@ def __init__(self, name, package_name, resource_name, resource_path): self.name = name self.value = f"{package_name}:{resource_name}" self._resource_path = resource_path + self._package_name = package_name + self._resource_name = resource_name def mock_entry_points(group=None): if group == "ndev_settings.manifest": @@ -109,23 +111,46 @@ def mock_entry_points(group=None): ] return [] - # Mock importlib.resources.files to return our test file - def mock_files(package_name): - if package_name == "mock_package": + # Mock distribution() to return our test file path + class MockPackagePath: + def __init__(self, package_name, resource_name, actual_path): + self._path = actual_path + self.name = resource_name # This is what file.name returns + self._package_name = package_name + + def __str__(self): + # Return something like "mock_package/settings.yaml" + # so package_name is in str(file) + return f"{self._package_name}/{self.name}" + + class MockDistribution: + def __init__(self, package_name, resource_name, resource_path): + self._package_name = package_name + self._resource_path = resource_path + # Create a mock files list with proper name and str representation + self.files = [ + MockPackagePath(package_name, resource_name, resource_path) + ] - class MockPath: - def __truediv__(self, resource_name): - if resource_name == "settings.yaml": - return external_file - return tmp_path / resource_name + def locate_file(self, file): + if hasattr(file, "_path"): + return file._path + # For editable install fallback + return self._resource_path.parent - return MockPath() - raise ImportError(f"No module named '{package_name}'") + from importlib.metadata import distribution as orig_dist + + def mock_distribution(package_name): + if package_name == "mock_package": + return MockDistribution( + package_name, "settings.yaml", external_file + ) + return orig_dist(package_name) # Apply the patches monkeypatch.setattr( "ndev_settings._settings.entry_points", mock_entry_points ) - monkeypatch.setattr("importlib.resources.files", mock_files) + monkeypatch.setattr("importlib.metadata.distribution", mock_distribution) return external_file diff --git a/tests/test_settings.py b/tests/test_settings.py index 398fc4f..b31a175 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -351,7 +351,7 @@ def test_duplicate_external_contributions_handling( yaml.dump(external2_data, default_flow_style=False) ) - # Mock multiple entry points using napari-style resource paths + # Mock multiple entry points class MockEntryPoint: def __init__( self, name, package_name, resource_name, resource_path @@ -378,29 +378,49 @@ def mock_entry_points(group=None): ] return [] - # Mock importlib.resources.files to return our test files - def mock_files(package_name): - class MockPath: - def __truediv__(self, resource_name): - if ( - package_name == "mock_package1" - and resource_name == "settings.yaml" - ): - return external1_file - elif ( - package_name == "mock_package2" - and resource_name == "settings.yaml" - ): - return external2_file - return tmp_path / resource_name - - return MockPath() + # Mock distribution() to return our test files + class MockPackagePath: + def __init__(self, package_name, resource_name, actual_path): + self._path = actual_path + self.name = resource_name + self._package_name = package_name + + def __str__(self): + return f"{self._package_name}/{self.name}" + + class MockDistribution: + def __init__(self, package_name, resource_name, resource_path): + self._package_name = package_name + self._resource_path = resource_path + self.files = [ + MockPackagePath(package_name, resource_name, resource_path) + ] + + def locate_file(self, file): + if hasattr(file, "_path"): + return file._path + return self._resource_path.parent + + from importlib.metadata import distribution as orig_dist + + def mock_distribution(package_name): + if package_name == "mock_package1": + return MockDistribution( + package_name, "settings.yaml", external1_file + ) + elif package_name == "mock_package2": + return MockDistribution( + package_name, "settings.yaml", external2_file + ) + return orig_dist(package_name) # Apply patches monkeypatch.setattr( "ndev_settings._settings.entry_points", mock_entry_points ) - monkeypatch.setattr("importlib.resources.files", mock_files) + monkeypatch.setattr( + "importlib.metadata.distribution", mock_distribution + ) # Load settings settings = Settings(str(test_settings_file)) From d7b3f95ab9818d8a9a496b0c675919bc00df8055 Mon Sep 17 00:00:00 2001 From: Tim Monko Date: Tue, 2 Dec 2025 12:59:29 -0600 Subject: [PATCH 07/13] update README with npe1 mention --- README.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/README.md b/README.md index 66b464b..1e28539 100644 --- a/README.md +++ b/README.md @@ -128,6 +128,18 @@ settings.reset_to_default(group="Canvas") # Reset entire group settings.reset_to_default() # Reset all settings ``` +## Performance Note: npe1 Plugin Compatibility + +If you have many legacy npe1 plugins installed (e.g., `napari-assistant`, `napari-segment-blobs-and-things-with-membranes`, `napari-simpleitk-image-processing`), you may experience slow widget loading times (10+ seconds) the first time you open the settings plugin widget in a napari session. This is a known issue in napari's npe1 compatibility layer, not specific to ndev-settings. The npe1 adapter iterates through all plugin widgets and performs expensive metadata lookups for each legacy plugin. + +**Workaround**: If you don't need npe1 runtime behavior plugins, you can disable the adapter in napari: + +1. Go to `File` -> `Preferences` -> `Plugins` +2. Uncheck "Use npe2 adaptor" +3. Restart napari + +This dramatically improves widget loading times since only pure npe2 plugins are discovered. + ## How Settings Persistence Works Settings are automatically cached to improve startup performance: From 8aaf35db651f71ee3bce999841772d077cfe6965 Mon Sep 17 00:00:00 2001 From: Tim Monko Date: Tue, 2 Dec 2025 14:41:31 -0600 Subject: [PATCH 08/13] revert to flat format, discover editables, and better tests --- src/ndev_settings/_cli.py | 19 +++++++++--- src/ndev_settings/_settings.py | 49 ++++++++++++++++++++++--------- tests/conftest.py | 10 +++++++ tests/test_cli.py | 53 +++++++++++++++++++++++++++++++++- 4 files changed, 113 insertions(+), 18 deletions(-) diff --git a/src/ndev_settings/_cli.py b/src/ndev_settings/_cli.py index a81240d..db29827 100644 --- a/src/ndev_settings/_cli.py +++ b/src/ndev_settings/_cli.py @@ -9,6 +9,9 @@ def reset_values_to_defaults(settings_file: Path | str) -> bool: """Reset all 'value' fields to match 'default' fields in a settings YAML file. + Works with flat format settings files where groups are at the top level. + Metadata keys starting with '_' (e.g., _entry_points_hash) are preserved. + Parameters ---------- settings_file : Path | str @@ -26,14 +29,20 @@ def reset_values_to_defaults(settings_file: Path | str) -> bool: return False with open(settings_file) as f: - settings = yaml.safe_load(f) + settings_data = yaml.load(f, Loader=yaml.FullLoader) - if not settings: + if not settings_data: return False modified = False - for group_name, group_settings in settings.items(): + for group_name, group_settings in settings_data.items(): + # Skip metadata keys (e.g., _entry_points_hash) + if group_name.startswith("_"): + continue + if not isinstance(group_settings, dict): + continue + for setting_name, setting_data in group_settings.items(): if ( isinstance(setting_data, dict) @@ -50,7 +59,9 @@ def reset_values_to_defaults(settings_file: Path | str) -> bool: if modified: with open(settings_file, "w") as f: - yaml.dump(settings, f, default_flow_style=False, sort_keys=False) + yaml.dump( + settings_data, f, default_flow_style=False, sort_keys=False + ) return modified diff --git a/src/ndev_settings/_settings.py b/src/ndev_settings/_settings.py index 95625d0..1b81806 100644 --- a/src/ndev_settings/_settings.py +++ b/src/ndev_settings/_settings.py @@ -116,10 +116,9 @@ def _load_defaults(self) -> dict: from importlib.metadata import distribution dist = distribution(package_name) - - # For installed packages, locate the resource file - # The package files are relative to the distribution location yaml_path = None + + # For regular installs, dist.files contains the file list for file in dist.files or []: if file.name == resource_name and package_name in str( file @@ -128,7 +127,31 @@ def _load_defaults(self) -> dict: break if yaml_path is None: - # Fallback: try direct path construction for editable installs + # For editable installs, check direct_url.json (PEP 610) + direct_url_file = dist._path / "direct_url.json" + if direct_url_file.exists(): + import json + + with open(direct_url_file) as f: + direct_url = json.load(f) + if direct_url.get("dir_info", {}).get("editable"): + # Editable install - use source path + url = direct_url["url"] + if url.startswith("file:///"): + source_path = Path(url[8:]) # Remove file:/// + elif url.startswith("file://"): + source_path = Path(url[7:]) + else: + source_path = Path(url) + yaml_path = ( + source_path + / "src" + / package_name + / resource_name + ) + + if yaml_path is None: + # Final fallback: try site-packages path package_location = str(dist.locate_file(package_name)) yaml_path = Path(package_location) / resource_name @@ -158,21 +181,23 @@ def _load_saved(self) -> dict | None: return None # Check if packages changed - if so, need to re-discover - saved_hash = saved.get("_entry_points_hash") + saved_hash = saved.pop("_entry_points_hash", None) if saved_hash != _get_entry_points_hash(): # Packages installed/removed - merge new defaults with saved values defaults = self._load_defaults() - saved_settings = saved.get("settings", {}) - merged = self._merge_with_saved(defaults, saved_settings) + merged = self._merge_with_saved(defaults, saved) self._save_settings(merged) return merged - return saved.get("settings") + return saved def _merge_with_saved(self, defaults: dict, saved: dict) -> dict: """Merge saved user values into fresh defaults.""" merged = {} for group_name, group_settings in defaults.items(): + # Skip metadata keys + if group_name.startswith("_"): + continue merged[group_name] = {} for name, data in group_settings.items(): merged[group_name][name] = data.copy() @@ -184,13 +209,11 @@ def _merge_with_saved(self, defaults: dict, saved: dict) -> dict: return merged def _save_settings(self, settings: dict) -> None: - """Save settings to file.""" + """Save settings to file in flat format with hash at top.""" try: _SETTINGS_DIR.mkdir(parents=True, exist_ok=True) - data = { - "_entry_points_hash": _get_entry_points_hash(), - "settings": settings, - } + # Flat format: hash first, then all settings groups + data = {"_entry_points_hash": _get_entry_points_hash(), **settings} with open(_SETTINGS_FILE, "w") as f: yaml.dump(data, f, default_flow_style=False, sort_keys=False) except (OSError, PermissionError) as e: diff --git a/tests/conftest.py b/tests/conftest.py index d0aca3d..35dcef5 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -13,6 +13,7 @@ def isolate_settings(tmp_path, monkeypatch, test_data_dir): This redirects both: 1. The settings file location (where user settings are saved) 2. The defaults file (to use test data instead of real package defaults) + 3. Entry points (to prevent external packages from contributing settings) This allows tests to run with real persistence behavior while being isolated. """ @@ -26,6 +27,15 @@ def isolate_settings(tmp_path, monkeypatch, test_data_dir): monkeypatch.setattr(_settings, "_SETTINGS_DIR", test_settings_dir) monkeypatch.setattr(_settings, "_SETTINGS_FILE", test_settings_file) + # Mock entry_points to return empty list (isolate from external packages) + from importlib.metadata import EntryPoints + + monkeypatch.setattr( + _settings, + "entry_points", + lambda group: EntryPoints([]), + ) + # Copy test data to use as defaults test_defaults_path = tmp_path / "test_defaults.yaml" shutil.copy(test_data_dir / "test_settings.yaml", test_defaults_path) diff --git a/tests/test_cli.py b/tests/test_cli.py index f2d1e97..9e44aa3 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -31,7 +31,7 @@ def test_reset_modified_values(self, tmp_path): assert result is True with open(settings_file) as f: - updated = yaml.safe_load(f) + updated = yaml.load(f, Loader=yaml.FullLoader) assert updated["TestGroup"]["setting1"]["value"] == "original" assert updated["TestGroup"]["setting2"]["value"] == 50 @@ -128,6 +128,57 @@ def test_accepts_string_path(self, tmp_path): assert result is True + def test_flat_format_with_entry_points_hash(self, tmp_path): + """Test handling of cached settings file with _entry_points_hash.""" + settings_file = tmp_path / "settings.yaml" + settings_file.write_text( + yaml.dump( + { + "_entry_points_hash": "abc123", + "TestGroup": { + "setting1": { + "value": "modified", + "default": "original", + }, + "setting2": {"value": 100, "default": 50}, + }, + } + ) + ) + + result = reset_values_to_defaults(settings_file) + + assert result is True + with open(settings_file) as f: + updated = yaml.load(f, Loader=yaml.FullLoader) + # Hash should be preserved + assert updated["_entry_points_hash"] == "abc123" + # Settings should be reset + assert updated["TestGroup"]["setting1"]["value"] == "original" + assert updated["TestGroup"]["setting2"]["value"] == 50 + + def test_skips_underscore_prefixed_keys(self, tmp_path): + """Test that underscore-prefixed keys are skipped, not treated as groups.""" + settings_file = tmp_path / "settings.yaml" + settings_file.write_text( + yaml.dump( + { + "_metadata": "some_value", # Should be skipped + "TestGroup": { + "setting1": { + "value": "modified", + "default": "original", + }, + }, + } + ) + ) + + # Should not raise an error when encountering non-dict group + result = reset_values_to_defaults(settings_file) + + assert result is True + class TestMainResetValues: """Tests for main_reset_values CLI entry point.""" From 17463655016b581c1913ec2dcb98844178776a2a Mon Sep 17 00:00:00 2001 From: Tim Monko Date: Tue, 2 Dec 2025 15:14:10 -0600 Subject: [PATCH 09/13] small style for if/else --- src/ndev_settings/_settings_widget.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/ndev_settings/_settings_widget.py b/src/ndev_settings/_settings_widget.py index 8fa5a4a..00d3ff1 100644 --- a/src/ndev_settings/_settings_widget.py +++ b/src/ndev_settings/_settings_widget.py @@ -45,8 +45,7 @@ def _create_widget_for_setting( for key, value in info.items(): if key in ["default", "value", "tooltip", "dynamic_choices"]: continue - else: - widget_options[key] = value + widget_options[key] = value # Handle dynamic choices if "dynamic_choices" in info: From 619e3a9d3f624b760ba338c5a6b16e1451e90bb3 Mon Sep 17 00:00:00 2001 From: Tim Monko Date: Tue, 2 Dec 2025 15:17:16 -0600 Subject: [PATCH 10/13] Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- src/ndev_settings/_settings.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ndev_settings/_settings.py b/src/ndev_settings/_settings.py index 1b81806..a241c1c 100644 --- a/src/ndev_settings/_settings.py +++ b/src/ndev_settings/_settings.py @@ -36,7 +36,7 @@ def _get_entry_points_hash() -> str: """ eps = entry_points(group="ndev_settings.manifest") ep_strings = sorted(f"{ep.name}:{ep.value}" for ep in eps) - return hashlib.md5("|".join(ep_strings).encode()).hexdigest() + return hashlib.sha256("|".join(ep_strings).encode()).hexdigest() def clear_settings() -> None: @@ -164,7 +164,7 @@ def _load_defaults(self) -> dict: for name, data in group_settings.items(): if name not in all_settings[group_name]: all_settings[group_name][name] = data - except (ModuleNotFoundError, FileNotFoundError, ValueError) as e: + except (ModuleNotFoundError, FileNotFoundError, ValueError, PermissionError, OSError) as e: logger.warning( "Failed to load settings from '%s': %s", ep.name, e ) From 1782f6156fee45e26c8f4beb2df46facadd2db75 Mon Sep 17 00:00:00 2001 From: Tim Monko Date: Tue, 2 Dec 2025 15:32:32 -0600 Subject: [PATCH 11/13] better testing of edge cases --- README.md | 2 +- tests/test_settings.py | 110 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 111 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 1e28539..4f5f0b7 100644 --- a/README.md +++ b/README.md @@ -135,7 +135,7 @@ If you have many legacy npe1 plugins installed (e.g., `napari-assistant`, `napar **Workaround**: If you don't need npe1 runtime behavior plugins, you can disable the adapter in napari: 1. Go to `File` -> `Preferences` -> `Plugins` -2. Uncheck "Use npe2 adaptor" +2. Uncheck "Use npe2 adapter" 3. Restart napari This dramatically improves widget loading times since only pure npe2 plugins are discovered. diff --git a/tests/test_settings.py b/tests/test_settings.py index b31a175..383c9af 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -498,3 +498,113 @@ def test_dynamic_choices(empty_settings_file): choices = settings.get_dynamic_choices("nonexistent.entry.point") assert isinstance(choices, list) assert len(choices) == 0 # Should return empty list when no entries found + + +class TestEdgeCases: + """Test edge cases and error handling.""" + + def test_build_groups_skips_metadata_keys(self, test_settings_file): + """Test that _build_groups skips underscore-prefixed keys.""" + settings = Settings(str(test_settings_file)) + + # Manually call _build_groups with settings containing metadata + settings_with_metadata = { + "_entry_points_hash": "abc123", + "_other_metadata": {"some": "data"}, + "ValidGroup": {"setting1": {"value": 10, "default": 10}}, + } + + # Clear existing groups and rebuild + settings._build_groups(settings_with_metadata) + + # Should have ValidGroup but not metadata keys as attributes + assert hasattr(settings, "ValidGroup") + assert not hasattr(settings, "_entry_points_hash") + assert not hasattr(settings, "_other_metadata") + + def test_save_failure_logs_warning( + self, test_settings_file, monkeypatch, caplog + ): + """Test that save failures are logged as warnings.""" + import logging + + settings = Settings(str(test_settings_file)) + + # Mock yaml.dump to raise when trying to save + def failing_dump(*args, **kwargs): + raise OSError("Cannot write to file") + + monkeypatch.setattr("yaml.dump", failing_dump) + + # Should not raise, but should log + with caplog.at_level(logging.WARNING): + settings._save_settings(settings._grouped_settings) + + assert "Failed to save settings" in caplog.text + + def test_editable_install_detection( + self, test_settings_file, tmp_path, monkeypatch + ): + """Test that editable installs are detected via direct_url.json.""" + import json + + from ndev_settings import _settings + + # Create a mock editable install structure + source_dir = tmp_path / "source" + source_dir.mkdir() + src_package_dir = source_dir / "src" / "mock_package" + src_package_dir.mkdir(parents=True) + yaml_file = src_package_dir / "settings.yaml" + yaml_file.write_text( + "Editable_Group:\n setting1:\n value: 42\n default: 42\n" + ) + + # Create a mock dist_info with direct_url.json + dist_info_path = tmp_path / "mock_package-1.0.0.dist-info" + dist_info_path.mkdir() + direct_url_file = dist_info_path / "direct_url.json" + direct_url_file.write_text( + json.dumps( + { + "url": f"file:///{source_dir.as_posix()}", + "dir_info": {"editable": True}, + } + ) + ) + + class MockEntryPoint: + def __init__(self): + self.name = "mock_editable" + self.value = "mock_package:settings.yaml" + + class MockDistribution: + def __init__(self): + self._path = dist_info_path + self.files = [] # Empty files list to trigger fallback + + def locate_file(self, file): + return tmp_path / "nonexistent" # Force fallback + + def mock_entry_points(group=None): + if group == "ndev_settings.manifest": + return [MockEntryPoint()] + return [] + + def mock_distribution(name): + if name == "mock_package": + return MockDistribution() + from importlib.metadata import distribution + + return distribution(name) + + monkeypatch.setattr(_settings, "entry_points", mock_entry_points) + monkeypatch.setattr( + "importlib.metadata.distribution", mock_distribution + ) + + settings = Settings(str(test_settings_file)) + + # Should have loaded the editable package's settings + assert hasattr(settings, "Editable_Group") + assert settings.Editable_Group.setting1 == 42 From 23c862568c73093af928a9f132f373eb7be66764 Mon Sep 17 00:00:00 2001 From: Tim Monko Date: Tue, 2 Dec 2025 16:30:47 -0600 Subject: [PATCH 12/13] better public api usage in file handling --- src/ndev_settings/_settings.py | 46 ++++++++++++++++++++++++++++------ tests/test_settings.py | 40 ++++++++++++++++++++--------- 2 files changed, 67 insertions(+), 19 deletions(-) diff --git a/src/ndev_settings/_settings.py b/src/ndev_settings/_settings.py index a241c1c..5f7df63 100644 --- a/src/ndev_settings/_settings.py +++ b/src/ndev_settings/_settings.py @@ -4,6 +4,7 @@ import logging from importlib.metadata import entry_points from pathlib import Path +from urllib.parse import unquote, urlparse import appdirs import yaml @@ -128,19 +129,38 @@ def _load_defaults(self) -> dict: if yaml_path is None: # For editable installs, check direct_url.json (PEP 610) - direct_url_file = dist._path / "direct_url.json" - if direct_url_file.exists(): + # Use locate_file for public API instead of private dist._path + try: + direct_url_file = Path( + str(dist.locate_file("direct_url.json")) + ) + except (AttributeError, TypeError): + direct_url_file = None + + if ( + direct_url_file is not None + and direct_url_file.exists() + ): import json with open(direct_url_file) as f: direct_url = json.load(f) if direct_url.get("dir_info", {}).get("editable"): # Editable install - use source path + # Use urllib.parse for proper file URL handling url = direct_url["url"] - if url.startswith("file:///"): - source_path = Path(url[8:]) # Remove file:/// - elif url.startswith("file://"): - source_path = Path(url[7:]) + parsed = urlparse(url) + if parsed.scheme == "file": + # Handle both Unix and Windows paths + path_str = unquote(parsed.path) + # On Windows, path may start with /C:/... + if ( + len(path_str) > 2 + and path_str[0] == "/" + and path_str[2] == ":" + ): + path_str = path_str[1:] # Remove leading / + source_path = Path(path_str) else: source_path = Path(url) yaml_path = ( @@ -164,7 +184,13 @@ def _load_defaults(self) -> dict: for name, data in group_settings.items(): if name not in all_settings[group_name]: all_settings[group_name][name] = data - except (ModuleNotFoundError, FileNotFoundError, ValueError, PermissionError, OSError) as e: + except ( + ModuleNotFoundError, + FileNotFoundError, + ValueError, + PermissionError, + OSError, + ) as e: logger.warning( "Failed to load settings from '%s': %s", ep.name, e ) @@ -181,6 +207,9 @@ def _load_saved(self) -> dict | None: return None # Check if packages changed - if so, need to re-discover + # Pop the hash from saved - this is safe because: + # - If hash doesn't match, we merge with defaults (saved used for values only) + # - If hash matches, we return saved (but _build_groups skips _ keys anyway) saved_hash = saved.pop("_entry_points_hash", None) if saved_hash != _get_entry_points_hash(): # Packages installed/removed - merge new defaults with saved values @@ -227,6 +256,9 @@ def _build_groups(self, settings: dict): # Skip metadata keys (e.g., _entry_points_hash) if group_name.startswith("_"): continue + # Skip malformed entries that aren't dicts + if not isinstance(group_settings, dict): + continue group_obj = SettingsGroup() for name, setting_data in group_settings.items(): if isinstance(setting_data, dict) and "value" in setting_data: diff --git a/tests/test_settings.py b/tests/test_settings.py index 383c9af..28ab1a1 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -142,31 +142,39 @@ def test_cached_load_uses_saved_file( self, test_settings_file, monkeypatch ): """Test that second load uses cached settings file, not _load_defaults.""" - # First load - discovers from files and saves - settings1 = Settings(str(test_settings_file)) + from ndev_settings import _settings - # Track if _load_defaults is called on second load - load_defaults_called = False + # Track all calls to _load_defaults + load_defaults_call_count = 0 original_load_defaults = Settings._load_defaults def tracking_load_defaults(self): - nonlocal load_defaults_called - load_defaults_called = True + nonlocal load_defaults_call_count + load_defaults_call_count += 1 return original_load_defaults(self) monkeypatch.setattr(Settings, "_load_defaults", tracking_load_defaults) - # Second load - should use cached file, not call _load_defaults + # First load - should discover from files (calls _load_defaults) + settings1 = Settings(str(test_settings_file)) + first_load_calls = load_defaults_call_count + + # Verify the cache file was created + assert ( + _settings._SETTINGS_FILE.exists() + ), "Cache file should exist after first load" + + # Second load - should use cached file, not call _load_defaults again settings2 = Settings(str(test_settings_file)) # Both should have same values assert settings1.Group_A.setting_int == settings2.Group_A.setting_int assert settings2._grouped_settings is not None - # _load_defaults should NOT have been called (cache was used) + # _load_defaults should have been called once (first load), but not on second assert ( - not load_defaults_called - ), "Expected cached path, but _load_defaults was called" + load_defaults_call_count == first_load_calls + ), f"Expected {first_load_calls} calls, got {load_defaults_call_count} - cache not working" class TestCachingBehavior: @@ -584,7 +592,10 @@ def __init__(self): self.files = [] # Empty files list to trigger fallback def locate_file(self, file): - return tmp_path / "nonexistent" # Force fallback + # Return the real direct_url.json, but nonexistent for other files + if file == "direct_url.json": + return direct_url_file + return tmp_path / "nonexistent" # Force fallback for others def mock_entry_points(group=None): if group == "ndev_settings.manifest": @@ -603,8 +614,13 @@ def mock_distribution(name): "importlib.metadata.distribution", mock_distribution ) + # Clear any cached settings to force fresh load + _settings.clear_settings() + settings = Settings(str(test_settings_file)) # Should have loaded the editable package's settings - assert hasattr(settings, "Editable_Group") + assert hasattr( + settings, "Editable_Group" + ), f"Missing Editable_Group. Attrs: {[a for a in dir(settings) if not a.startswith('_')]}" assert settings.Editable_Group.setting1 == 42 From 332046affe014532e4bb8e5f3cbc0647eb0a8f39 Mon Sep 17 00:00:00 2001 From: Tim Monko Date: Tue, 2 Dec 2025 16:46:35 -0600 Subject: [PATCH 13/13] fix editable path --- src/ndev_settings/_settings.py | 13 ++++++------- tests/test_settings.py | 23 ++++++++++++++++++++--- 2 files changed, 26 insertions(+), 10 deletions(-) diff --git a/src/ndev_settings/_settings.py b/src/ndev_settings/_settings.py index 5f7df63..104d771 100644 --- a/src/ndev_settings/_settings.py +++ b/src/ndev_settings/_settings.py @@ -129,13 +129,12 @@ def _load_defaults(self) -> dict: if yaml_path is None: # For editable installs, check direct_url.json (PEP 610) - # Use locate_file for public API instead of private dist._path - try: - direct_url_file = Path( - str(dist.locate_file("direct_url.json")) - ) - except (AttributeError, TypeError): - direct_url_file = None + # Find direct_url.json from dist.files (includes dist-info path) + direct_url_file = None + for file in dist.files or []: + if file.name == "direct_url.json": + direct_url_file = Path(str(dist.locate_file(file))) + break if ( direct_url_file is not None diff --git a/tests/test_settings.py b/tests/test_settings.py index 28ab1a1..756b5d9 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -586,14 +586,31 @@ def __init__(self): self.name = "mock_editable" self.value = "mock_package:settings.yaml" + class MockFile: + """Mock file object that mimics PackagePath behavior.""" + + def __init__(self, path): + self._path = path + + @property + def name(self): + return self._path.split("/")[-1] + + def __str__(self): + return self._path + class MockDistribution: def __init__(self): self._path = dist_info_path - self.files = [] # Empty files list to trigger fallback + # Include direct_url.json in the files list (like real dist-info) + self.files = [ + MockFile("mock_package-1.0.0.dist-info/direct_url.json") + ] def locate_file(self, file): - # Return the real direct_url.json, but nonexistent for other files - if file == "direct_url.json": + # Handle both string and MockFile input + file_str = str(file) + if "direct_url.json" in file_str: return direct_url_file return tmp_path / "nonexistent" # Force fallback for others