-
-
Notifications
You must be signed in to change notification settings - Fork 403
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
plugin: new interface #1479
plugin: new interface #1479
Conversation
This PR is kind of related to #1471, as it modifies Sopel's internals, and it will definitively makes CLI tools easier to make. For instance, it'll be much easier to implement a "list all plugins" CLI tool, or "configure this module in particular", etc. |
Edit: fixed! Meh. Travis found a kind of cyclic import. Fine. I'll have to fix that one way or another. That'll be for tomorrow! |
6b1fb49
to
166bb8b
Compare
As said on IRC:
So I need to update |
As said on IRC: I merged #1490 in, so this PR will handle everything related to plugin's loading & reloading. Also: so many things to fix, make, improve, and else. 😄 |
2b3b760
to
bdfc89a
Compare
I've figure out a way to:
but I didn't add tests for sopel.plugins, which is not good, and I need to fix that. And then, eventually, I'll ask for a review! |
d31ac46
to
ab8f174
Compare
Yup, it does. It's not perfect, because I do not handle (yet) sub-module reloading, but that'll come in later. |
So, for testing purpose, I need #1557 to be merged: I want to test the startup routine of the bot, when it loads plugins. And for that, I need to be able to stop the JobScheduler, otherwise the test suite never stop running. That could be annoying. |
158b729
to
93ffdb5
Compare
4aeaf27
to
9de1d9e
Compare
Skip until sopel-irc#1557 is merged
Co-Authored-By: Exirel <[email protected]>
Co-Authored-By: dgw <[email protected]>
If `bot.reply` does throw an exception, it would have cause the bot to send the wrong message, an error that would not be related to the actual loading of the plugin. Co-Authored-By: dgw <[email protected]>
Co-Authored-By: dgw <[email protected]>
52a0d5d
to
b0e7bfa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I didn't go through the whole diff again, just a patchwork of changes made since my last review. But this is "core" enough to Sopel's operation that, once merged, any issues will probably bubble up very quickly as we all work on other stuff.
I'd like a review from one other @sopel-irc/rockstars before merging, so hopefully someone else is up to the task! 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up coming across some issues while testing this PR, but it's all related to the CLI (I'll put these in another issue). The plugin interface, however, looks good to me 🚀 (reload
not testing given all the issues it has now anyway... can't be worse 😅, so it may be better to get this in, then re-frame the reload issues in the context of this new interface).
What kind of issues? 😮 |
I trust @HumorBaby to open that other issue when he's ready. Unlike some of us (*cough* me *cough*), he's good at remembering what he promised to do. 😁 |
I was playing for a long time with the idea of a unified Sopel's Plugin interface: a way to abstract how plugins are loaded from the core, so that you can implement various way of finding and loading them without having to change how the bot behave and interact with commands, rules, events, etc.
This PR slightly changes how callables are registered, and totally change how they are reloaded.
Plugin Interface
The Plugin Interface is represented by the
sopel.plugins.handlers.AbstractPluginHandler
:plugin.load()
: load the plugin insys.modules
plugin.reload()
: reload the pluginplugin.configure(settings)
: handle the configuration wizard for the plugin (if necessary)plugin.setup(bot)
: setup the plugin (if necessary)plugin.register(bot)
: register the plugin with the botplugin.shutdown(bot)
: take action on shutdown (if necessary)plugin.is_loaded()
: tell if the plugin is loadedplugin.has_*()
(configure, setup, shutdown): tell if the plugin has this particular methodUnder the hood, I kept a lot of what is in
sopel.loader
, so it isn't a change that big!What's next?
I would like to add this kind of plugin:
sopel_modules.*
),but that'll be for another day!
Related issues, PR, and future works
sopel-*
CLI tools: this interface will be very useful to inspect plugins, show their details, configure and enable/disable them (as in Enhancement: Configuring individual modules only? #1519)time.sleep()
: with the rework of the JobScheduler and the new cycle of "shutdown, reload, register" that is better, it makes the JobScheduler and theinterval
decorator a good option for some cases (as requested by remind: use module.interval instead of custom monitoring #1581 or seen in Jobs weird on reload? #831)