NPE1Shim Part 1 - updated _from_npe1 conversion logic to prepare for …

…locally defined objects (#124)

* changes to _from_npe1 required for shim conversion

* add tests

* add magicfactory

* Empty to trigger tests coverage

Co-authored-by: Matthias Bussonnier <[email protected]>

npe2/manifest/utils.py

@@ -10,18 +10,23 @@
     Dict,
     Generic,
     Optional,
+    Sequence,
     SupportsInt,
     Tuple,
     TypeVar,
     Union,
 )
 
+if TYPE_CHECKING:
+    from npe2.manifest.schema import PluginManifest
+
 from ..types import PythonName
 
 if TYPE_CHECKING:
     from typing_extensions import Protocol
 
     from .._command_registry import CommandRegistry
+    from .contributions import ContributionPoints
 
     class ProvidesCommand(Protocol):
         command: str
@@ -31,6 +36,7 @@ def get_callable(self, _registry: Optional[CommandRegistry] = None):
 
 
 R = TypeVar("R")
+SHIM_NAME_PREFIX = "__npe1shim__."
 
 
 # TODO: add ParamSpec when it's supported better by mypy
@@ -153,16 +159,131 @@ def __str__(self) -> str:
         return v
 
 
-def import_python_name(python_name: PythonName) -> Any:
+def _import_npe1_shim(shim_name: str) -> Any:
+    """Import npe1 shimmed python_name
+
+    Some objects returned by npe1 hooks (such as locally defined partials or other
+    objects) don't have globally accessible python names. In such cases, we create
+    a "shim" python_name of the form:
+
+    `__npe1shim__.<hook_python_name>_<index>`
+
+    The implication is that the hook should be imported, called, and indexed to return
+    the corresponding item in the hook results.
+
+    Parameters
+    ----------
+    shim_name : str
+        A string in the form `__npe1shim__.<hook_python_name>_<index>`
+
+    Returns
+    -------
+    Any
+        The <index>th object returned from the callable <hook_python_name>.
+
+    Raises
+    ------
+    IndexError
+        If len(<hook_python_name>()) <= <index>
+    """
+
+    assert shim_name.startswith(SHIM_NAME_PREFIX), f"Invalid shim name: {shim_name}"
+    python_name, idx = shim_name[13:].rsplit("_", maxsplit=1)  # TODO, make a function
+    index = int(idx)
+
+    hook = import_python_name(python_name)
+    result = hook()
+    if isinstance(result, dict):
+        # things like sample_data hookspec return a dict, in which case we want the
+        # "idxth" item in the dict (assumes ordered dict, which is safe now)
+        result = list(result.values())
+    if not isinstance(result, list):
+        result = [result]  # pragma: no cover
+
+    try:
+        out = result[index]
+    except IndexError as e:  # pragma: no cover
+        raise IndexError(f"invalid npe1 shim index {index} for hook {hook}") from e
+
+    if "dock_widget" in python_name and isinstance(out, tuple):
+        return out[0]
+    if "sample_data" in python_name and isinstance(out, dict):
+        # this was a nested sample data
+        return out.get("data")
+
+    return out
+
+
+def import_python_name(python_name: Union[PythonName, str]) -> Any:
     from importlib import import_module
 
-    from ._validators import PYTHON_NAME_PATTERN
+    from . import _validators
 
-    match = PYTHON_NAME_PATTERN.match(python_name)
-    if not match:  # pragma: no cover
-        raise ValueError(f"Invalid python name: {python_name}")
+    if python_name.startswith(SHIM_NAME_PREFIX):
+        return _import_npe1_shim(python_name)
 
-    module_name, funcname = match.groups()
+    _validators.python_name(python_name)  # shows the best error message
+    match = _validators.PYTHON_NAME_PATTERN.match(python_name)
+    module_name, funcname = match.groups()  # type: ignore [union-attr]
 
     mod = import_module(module_name)
     return getattr(mod, funcname)
+
+
+def deep_update(dct: dict, merge_dct: dict, copy=True) -> dict:
+    """Merge possibly nested dicts"""
+    from copy import deepcopy
+
+    _dct = deepcopy(dct) if copy else dct
+    for k, v in merge_dct.items():
+        if k in _dct and isinstance(dct[k], dict) and isinstance(v, dict):
+            deep_update(_dct[k], v, copy=False)
+        elif isinstance(v, list):
+            if k not in _dct:
+                _dct[k] = []
+            _dct[k].extend(v)
+        else:
+            _dct[k] = v
+    return _dct
+
+
+def merge_manifests(manifests: Sequence[PluginManifest]):
+    from npe2.manifest.schema import PluginManifest
+
+    if not manifests:
+        raise ValueError("Cannot merge empty sequence of manifests")
+    if len(manifests) == 1:
+        return manifests[0]
+
+    assert len({mf.name for mf in manifests}) == 1, "All manifests must have same name"
+    assert (
+        len({mf.package_version for mf in manifests}) == 1
+    ), "All manifests must have same version"
+    assert (
+        len({mf.display_name for mf in manifests}) == 1
+    ), "All manifests must have same display_name"
+
+    mf0 = manifests[0]
+    info = mf0.dict(exclude={"contributions"}, exclude_unset=True)
+    info["contributions"] = merge_contributions([m.contributions for m in manifests])
+    return PluginManifest(**info)
+
+
+def merge_contributions(contribs: Sequence[Optional[ContributionPoints]]) -> dict:
+    _contribs = [c for c in contribs if c and c.dict(exclude_unset=True)]
+    if not _contribs:
+        return {}  # pragma: no cover
+
+    out = _contribs[0].dict(exclude_unset=True)
+    if len(_contribs) > 1:
+        for n, ctrb in enumerate(_contribs[1:]):
+            c = ctrb.dict(exclude_unset=True)
+            for cmd in c.get("commands", ()):
+                cmd["id"] = cmd["id"] + f"_{n + 2}"
+            for name, val in c.items():
+                if isinstance(val, list):
+                    for item in val:
+                        if "command" in item:
+                            item["command"] = item["command"] + f"_{n + 2}"
+            out = deep_update(out, c)
+    return out

tests/conftest.py

@@ -1,11 +1,17 @@
 import sys
+from importlib import abc
 from pathlib import Path
 from unittest.mock import patch
 
 import pytest
 
 from npe2 import PluginManager, PluginManifest
 
+try:
+    from importlib import metadata
+except ImportError:
+    import importlib_metadata as metadata  # type: ignore
+
 
 @pytest.fixture
 def sample_path():
@@ -20,10 +26,12 @@ def sample_manifest(sample_path):
 @pytest.fixture
 def uses_sample_plugin(sample_path):
     sys.path.append(str(sample_path))
-    pm = PluginManager.instance()
-    pm.discover()
-    yield
-    sys.path.remove(str(sample_path))
+    try:
+        pm = PluginManager.instance()
+        pm.discover()
+        yield
+    finally:
+        sys.path.remove(str(sample_path))
 
 
 @pytest.fixture
@@ -55,6 +63,30 @@ def npe1_repo():
     return Path(__file__).parent / "npe1-plugin"
 
 
+@pytest.fixture
+def uses_npe1_plugin(npe1_repo):
+    import site
+
+    class Importer(abc.MetaPathFinder):
+        def find_spec(self, *_, **__):
+            return None
+
+        def find_distributions(self, ctx, **k):
+            if ctx.name == "npe1-plugin":
+                pth = npe1_repo / "npe1-plugin-0.0.1.dist-info"
+                yield metadata.PathDistribution(pth)
+            return
+
+    sys.meta_path.append(Importer())
+    sys.path.append(str(npe1_repo))
+    try:
+        pkgs = site.getsitepackages() + [str(npe1_repo)]
+        with patch("site.getsitepackages", return_value=pkgs):
+            yield
+    finally:
+        sys.path.remove(str(npe1_repo))
+
+
 @pytest.fixture
 def npe1_plugin_module(npe1_repo):
     import sys
@@ -74,23 +106,39 @@ def npe1_plugin_module(npe1_repo):
 
 @pytest.fixture
 def mock_npe1_pm():
-    from napari_plugin_engine import PluginManager
-
-    from npe2._from_npe1 import HookSpecs
+    from napari_plugin_engine import PluginManager, napari_hook_specification
+
+    # fmt: off
+    class HookSpecs:
+        def napari_provide_sample_data(): ...  # type: ignore  # noqa: E704
+        def napari_get_reader(path): ...  # noqa: E704
+        def napari_get_writer(path, layer_types): ...  # noqa: E704
+        def napari_write_image(path, data, meta): ...  # noqa: E704
+        def napari_write_labels(path, data, meta): ...  # noqa: E704
+        def napari_write_points(path, data, meta): ...  # noqa: E704
+        def napari_write_shapes(path, data, meta): ...  # noqa: E704
+        def napari_write_surface(path, data, meta): ...  # noqa: E704
+        def napari_write_vectors(path, data, meta): ...  # noqa: E704
+        def napari_experimental_provide_function(): ...  # type: ignore  # noqa: E704
+        def napari_experimental_provide_dock_widget(): ...  # type: ignore  # noqa: E704
+        def napari_experimental_provide_theme(): ...  # type: ignore  # noqa: E704
+    # fmt: on
+
+    for m in dir(HookSpecs):
+        if m.startswith("napari"):
+            setattr(HookSpecs, m, napari_hook_specification(getattr(HookSpecs, m)))
 
     pm = PluginManager("napari")
     pm.add_hookspecs(HookSpecs)
 
-    with patch("npe2._from_npe1.npe1_plugin_manager", new=lambda: (pm, (1, []))):
-        yield pm
+    yield pm
 
 
 @pytest.fixture
-def mock_npe1_pm_with_plugin(npe1_repo, mock_npe1_pm, npe1_plugin_module):
+def mock_npe1_pm_with_plugin(npe1_repo, npe1_plugin_module):
     """Mocks a fully installed local repository"""
     from npe2._from_npe1 import metadata, plugin_packages
 
-    mock_npe1_pm.register(npe1_plugin_module, "npe1-plugin")
     mock_dist = metadata.PathDistribution(npe1_repo / "npe1-plugin-0.0.1.dist-info")
 
     def _dists():

tests/npe1-plugin/npe1-plugin-0.0.1.dist-info/METADATA

@@ -1,2 +1,3 @@
 Metadata-Version: 2.1
 Name: npe1-plugin
+Version: 0.1.0

tests/npe1-plugin/npe1_module/__init__.py

@@ -1,3 +1,6 @@
+from functools import partial
+
+import numpy as np
 from magicgui import magic_factory
 from napari_plugin_engine import napari_hook_implementation
 
@@ -33,11 +36,16 @@ def napari_write_labels(path, data, meta):
 def napari_provide_sample_data():
     return {
         "random data": gen_data,
+        "local data": partial(np.ones, (4, 4)),
         "random image": "https://picsum.photos/1024",
         "sample_key": {
             "display_name": "Some Random Data (512 x 512)",
             "data": gen_data,
         },
+        "local_ones": {
+            "display_name": "Some local ones",
+            "data": partial(np.ones, (4, 4)),
+        },
     }
 
 
@@ -65,11 +73,25 @@ def napari_experimental_provide_theme():
     }
 
 
+factory = magic_factory(some_function)
+
+
 @napari_hook_implementation
 def napari_experimental_provide_dock_widget():
-    return [MyWidget, (magic_factory(some_function), {"name": "My Other Widget"})]
+    @magic_factory
+    def local_widget(y: str):
+        ...
+
+    return [
+        MyWidget,
+        (factory, {"name": "My Other Widget"}),
+        (local_widget, {"name": "Local Widget"}),
+    ]
 
 
 @napari_hook_implementation
 def napari_experimental_provide_function():
-    return some_function
+    def local_function(x: int):
+        ...
+
+    return [some_function, local_function]

tests/npe1-plugin/setup.cfg

@@ -1,5 +1,6 @@
 [metadata]
 name = npe1-plugin
+version = 0.1.0
 
 [options.entry_points]
 napari.plugin =

tests/test_cli.py

@@ -63,7 +63,7 @@ def test_cli_convert_repo_dry_run(npe1_repo, mock_npe1_pm_with_plugin):
 def test_cli_convert_svg():
     result = runner.invoke(app, ["convert", "napari-svg"])
     assert "Some issues occured:" in result.stdout
-    assert "Found a multi-layer writer, but it's not convertable" in result.stdout
+    assert "Found a multi-layer writer in 'napari-svg'" in result.stdout
     assert result.exit_code == 0