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

Update register_all #840

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Update register_all #840

wants to merge 1 commit into from

Conversation

cfuselli
Copy link
Member

If you do register_all on a module that contains a plugin that inherits with another, the effect of registering the plugin will be cancelled.

Example:

Module1:

class ParentPlugin(strax.Plugin):
    provides = "some_type"
    ...
    ...

Module 2:

class MyPlugin(ParentPlugin):
    ...
    ...

Running register_all on Module2 will register:

  • ParentPlugin
  • MyPlugin
  • ParentPlugin

Adding this check, this will only register MyPlugin.

Change needed in case of plugins that inherit from other plugins. The parent class would be registered once more after the plugin, basically cancelling the registration of the desired plugin.
@cfuselli
Copy link
Member Author

I see that some tests fail. Maybe I am missing something..

@JoranAngevaare
Copy link
Contributor

Seems like this "feature" was utilized in straxen in the past, so probably in other places too. Actually, in straxen we were particularly lazy as we register_all straxen.plugins: https://github.com/XENONnT/straxen/blob/fe9b9fae529862c6dc1e3e273f441d662bc7e355/straxen/contexts.py#L95. This means that probably close to none of the plugins will be registered after this change. You would have to change this to register_all([straxen.plugins.afterpulses.afterpulse_processing, straxen.plugins.aqmon_hits.aqmon_hits, ...]) etc.

Maybe instead for now you could add a warning that these additional plugins will not be registered anymore in a feature release unless explicitly imported? Or you could add a "strict/sloppy" argument for register all if yo want to be able to turn on/off this behavior. Otherwise things start breaking all at once (as you see with these tests now).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants