Fix naming of npe1 converted contribution commands and add tests (#374)

Prior to this PR, declaring multiple implementations of the same
hookspec in an npe1 plugin would lead to a converted manifest with
multiple commands of the same ID e.g. `plugin_name.get_reader` multiple
times, or `plugin_name.write_labels` multiple times.

Even though this is technically "valid" per the schema, it should not
be, because this leads to errors in trying to register the commands in
napari. App-model (correctly, imo), does not allow you to register
multiple commands with the same ID.

We should update the schema validator to disallow multiple declarations
of the same command ID, but in the meantime, this PR disambiguates
converted commands for the same hook spec by using the function name
instead of the spec name.

---------

Co-authored-by: Draga Doncila <[email protected]>

Author: DragaDoncila

src/npe2/_inspection/_from_npe1.py

@@ -404,11 +404,11 @@ def _parse_writer(self, impl: HookImplementation, layer: str):
         )
 
     def add_command(self, impl: HookImplementation, py_name: str = "") -> str:
-        name = impl.specname.replace("napari_", "")
-        id = f"{self.package}.{name}"
-        title = " ".join(name.split("_")).title()
         if not py_name:
             py_name = _python_name(impl.function)
+        name = impl.function.__name__
+        id = f"{self.package}.{name}"
+        title = " ".join(name.split("_")).title()
         c = CommandContribution(id=id, python_name=py_name, title=title)
         self.contributions["commands"].append(c)
         return id

tests/npe1-plugin/npe1_module/__init__.py

@@ -18,6 +18,10 @@ def gen_data(): ...
 def napari_get_reader(path): ...
 
 
+@napari_hook_implementation(specname="napari_get_reader")
+def napari_other_reader(path): ...
+
+
 @napari_hook_implementation
 def napari_write_image(path, data, meta): ...
 
@@ -26,6 +30,10 @@ def napari_write_image(path, data, meta): ...
 def napari_write_labels(path, data, meta): ...
 
 
+@napari_hook_implementation(specname="napari_write_labels")
+def napari_other_write_labels(path, data, meta): ...
+
+
 @napari_hook_implementation
 def napari_provide_sample_data():
     return {

tests/test_conversion.py

@@ -25,6 +25,32 @@ def test_conversion_from_module(mock_npe1_pm, npe1_plugin_module):
     assert isinstance(mf.dict(), dict)
 
 
+@pytest.mark.filterwarnings("ignore:Failed to convert napari_provide_sample_data")
+@pytest.mark.filterwarnings("ignore:Error converting function")
+@pytest.mark.filterwarnings("ignore:Error converting dock widget")
+def test_conversion_multiple_readers(mock_npe1_pm, npe1_plugin_module):
+    mf = manifest_from_npe1(module=npe1_plugin_module)
+    assert isinstance(mf.dict(), dict)
+    assert (readers := mf.contributions.readers) is not None
+    reader_commands = {r.command for r in readers}
+    assert len(reader_commands) == 2
+    assert "dynamic.napari_get_reader" in reader_commands
+    assert "dynamic.napari_other_reader" in reader_commands
+
+
+@pytest.mark.filterwarnings("ignore:Failed to convert napari_provide_sample_data")
+@pytest.mark.filterwarnings("ignore:Error converting function")
+@pytest.mark.filterwarnings("ignore:Error converting dock widget")
+def test_conversion_multiple_writers(mock_npe1_pm, npe1_plugin_module):
+    mf = manifest_from_npe1(module=npe1_plugin_module)
+    assert isinstance(mf.dict(), dict)
+    assert (writers := mf.contributions.writers) is not None
+    writer_commands = {r.command for r in writers}
+    assert len(writer_commands) == 3
+    assert "dynamic.napari_write_labels" in writer_commands
+    assert "dynamic.napari_other_write_labels" in writer_commands
+
+
 def test_conversion_from_obj_with_locals(mock_npe1_pm):
     from napari_plugin_engine import napari_hook_implementation