plugins: actually load plugins in load_plugins()

Instead of the first call to find_plugins(). This could change behavior
slightly, since plugin load (and errors that happen in the process) could
occur earlier now. But:

- I'd argue this is better anyway, since plugin load errors now happen
  in the initial call to load_plugins() instead of somewhere in the
  middle of the first operation that uses plugins.
- In our ui code, both happen immediately after each other anyway:
  ui._setup calls
  	1) ui._load_plugins -> plugins.load_plugins
	2) plugins.types, plugins.named_queries, ...

Author: wisp3rwind

beets/plugins.py

@@ -47,7 +47,7 @@
 
 
 if TYPE_CHECKING:
-    from collections.abc import Iterable
+    from collections.abc import Iterable, Iterator
 
     from confuse import ConfigView
 
@@ -342,7 +342,7 @@ def helper(func: TFunc[Item]) -> TFunc[Item]:
         return helper
 
 
-_classes: set[type[BeetsPlugin]] = set()
+_instances: dict[type[BeetsPlugin], BeetsPlugin] = {}
 
 
 def load_plugins(names: Sequence[str] = ()) -> None:
@@ -351,6 +351,8 @@ def load_plugins(names: Sequence[str] = ()) -> None:
     package in sys.path; the module indicated should contain the
     BeetsPlugin subclasses desired.
     """
+    classes = set()
+
     for name in names:
         try:
             mod = import_module(f".{name}", package=PLUGIN_NAMESPACE)
@@ -365,43 +367,39 @@ def load_plugins(names: Sequence[str] = ()) -> None:
             )
             continue
 
-        for _name, obj in inspect.getmembers(mod, inspect.isclass):
+        for _name, cls in inspect.getmembers(mod, inspect.isclass):
             if (
-                issubclass(obj, BeetsPlugin)
-                and obj != BeetsPlugin
-                and obj != MetadataSourcePlugin
+                issubclass(cls, BeetsPlugin)
+                and cls != BeetsPlugin
+                and cls != MetadataSourcePlugin
+                and cls not in _instances
             ):
-                _classes.add(obj)
+                classes.add(cls)
 
+    for cls in classes:
+        _register_plugin(cls)
 
-_instances: dict[type[BeetsPlugin], BeetsPlugin] = {}
 
+def _register_plugin(cls: type[BeetsPlugin]) -> None:
+    if cls in _instances:
+        return
 
-def find_plugins() -> list[BeetsPlugin]:
-    """Returns a list of BeetsPlugin subclass instances from all
-    currently loaded beets plugins. Loads the default plugin set
-    first.
+    try:
+        _instances[cls] = cls()
+    except Exception:
+        log.error(
+            "failed to initialize plugin class {}.{}",
+            cls.__module__,
+            cls.__qualname__,
+        )
+        raise
+
+
+def find_plugins() -> Iterator[BeetsPlugin]:
+    """Returns an iterator of BeetsPlugin subclass instances from all
+    currently loaded beets plugins.
     """
-    if _instances:
-        # After the first call, use cached instances for performance reasons.
-        # See https://github.com/beetbox/beets/pull/3810
-        return list(_instances.values())
-
-    plugins = []
-    for cls in _classes:
-        # Only instantiate each plugin class once.
-        if cls not in _instances:
-            try:
-                _instances[cls] = cls()
-            except Exception:
-                log.error(
-                    "failed to initialize plugin class {}.{}",
-                    cls.__module__,
-                    cls.__qualname__,
-                )
-                raise
-        plugins.append(_instances[cls])
-    return plugins
+    yield from _instances.values()
 
 
 # Communication with plugins.

beets/test/helper.py

@@ -487,7 +487,6 @@ def unload_plugins(self) -> None:
         for plugin_class in beets.plugins._instances:
             plugin_class.listeners = None
         self.config["plugins"] = []
-        beets.plugins._classes = set()
         beets.plugins._instances = {}
         Item._types = self.original_item_types
         Album._types = self.original_album_types

test/test_plugins.py

@@ -37,30 +37,22 @@
 
 
 class PluginLoaderTestCase(BasePluginTestCase):
-    def setup_plugin_loader(self):
-        # FIXME the mocking code is horrific, but this is the lowest and
-        # earliest level of the plugin mechanism we can hook into.
-        self._plugin_loader_patch = patch("beets.plugins.load_plugins")
-        self._plugin_classes = set()
-        load_plugins = self._plugin_loader_patch.start()
+    """A test case that adds an additional plugin to beets' plugin method
+    resolution mechanism.
 
-        def myload(names=()):
-            plugins._classes.update(self._plugin_classes)
-
-        load_plugins.side_effect = myload
-
-    def teardown_plugin_loader(self):
-        self._plugin_loader_patch.stop()
+    It does _not_ go through the full `plugins.load_plugins` code, which would
+    require that the plugin_class is part of an importable module.
+    """
 
     def register_plugin(self, plugin_class):
-        self._plugin_classes.add(plugin_class)
+        plugins._register_plugin(plugin_class)
 
     def setUp(self):
-        self.setup_plugin_loader()
         super().setUp()
+        self._prev_plugin_instances = plugins._instances.copy()
 
     def tearDown(self):
-        self.teardown_plugin_loader()
+        plugins._instances = self._prev_plugin_instances
         super().tearDown()
 
 

test/test_ui.py

@@ -1059,7 +1059,7 @@ def test_cli_config_file_loads_plugin_commands(self):
             file.write("plugins: test")
 
         self.run_command("--config", self.cli_config_path, "plugin", lib=None)
-        assert plugins.find_plugins()[0].is_test_plugin
+        assert next(iter(plugins.find_plugins())).is_test_plugin
         self.unload_plugins()
 
     def test_beetsdir_config(self):