Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds custom list of plugins to be passed on load #2481

Merged
merged 3 commits into from
Apr 5, 2024

Conversation

CyclingNinja
Copy link
Contributor

@CyclingNinja CyclingNinja commented Mar 28, 2024

Adds customisable list of plugins on load

Adds an argument to load_plugins in main that allows a custom list to be passed.
On empty, defaults to required plugins which have been added to _plugin_helpers

Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good so far, but think we need to tweak the logic related to requiring plugins - see below. API-wise I think the keyword argument to load_plugins should probably just be called names so that it would look like:

load_plugins(names=['glue.core.something'])

rather than

load_plugins(plugins_to_load=['glue.core.something'])

which is a bit repetitive. Internally we could still use plugins_to_load though.

glue/main.py Outdated
@@ -45,17 +49,26 @@ def load_plugins(splash=None, require_qt_plugins=False):
config = PluginConfig.load()

n_plugins = len(list(iter_plugin_entry_points()))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be best to actually define n_plugins as len(plugins_to_use) once plugins_to_use has been defined.

glue/main.py Outdated
if require_qt_plugins:
plugins_to_use = [*REQUIRED_PLUGINS, *REQUIRED_PLUGINS_QT]
else:
plugins_to_use = REQUIRED_PLUGINS
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think plugins_to_use should be based on the REQUIRED_* variables - these are just used to raise exceptions below if they are not present. I think we actually need two different variables inside this function - plugins_to_load(perhaps a better name than plugins_to_use) and plugins_to_require. I think the logic should be:

  • If plugins_to_load is not defined by the user, then plugins_to_load defaults to all entry points (iter_plugin_entry_points) and plugins_to_require defaults to either REQUIRED_PLUGINS or the combination of REQUIRED_PLUGINS and REQUIRED_PLUGINS_QT
  • If plugins_to_load is defined by the user, then plugins_to_require should just be set to plugins_to_load because if the user is explicitly asking for them we should error if they can't be loaded

@@ -90,7 +103,7 @@ def load_plugins(splash=None, require_qt_plugins=False):
_loaded_plugins.add(item.module)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't easily comment on lines above but based on my previous comment,

     if item.module in REQUIRED_PLUGINS:
                raise
            elif item.module in REQUIRED_PLUGINS_QT and require_qt_plugins:
                raise

should then become:

     if item.module in plugins_to_require:
                raise

Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks! In a follow-up PR, could you tweak the API of load_plugins so that the kwarg is called names instead of plugins_to_load? (see above comment about this).

@astrofrog astrofrog merged commit 9ea3636 into glue-viz:main Apr 5, 2024
22 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants