kdienes
commented
1 week ago I'm reasonably confident of this but haven't put in the work to be 100%:
load_plugin() always takes its spec from _parse_plugin_spec
the check proposed is only when spec['type']=='module', i.e., when we are adding a plugin from a python module
so I think you're right that it doesn't get engaged for paths or git repositories
It's not easy to have the check in handle_module_spec() as written, because handle_module_spec() takes a importlib.machinery.ModuleSpec and not a name, making it harder to get a good error message.
I think handle_module_spec() is also called from handle_path_url(), which works like you describe
I did handle_module_spec() the way I did to lessen the change from what was there before, but we could probably come up with something easier to understand if we put some time into it.
Tangentially, I regret using the name 'plugin spec' adjacent to 'module spec'
@kdienes What happens in the case where ignore_import-errors is True? Do we introduce an error we shouldn't? ignore_import-errors gets set in install_dependencies and gen_requirements; the intent is to go recurse through all the plugins and load at least their metadata so we can collect their dependencies. does the code you are modifying only get called when a plugin is specified as a module, or does it get called indirectly for path and git specs?
Similarly, at least for path, I think we allow metadata-only plugins.
I think the answer is that this code is safe and only affects plugins specified by module, but want a second opinion.