Implement PluginManager class #212
Conversation
|
Any suggestion on how to workaround the type checking failures in |
|
@unode thanks for putting in this effort to improve the bot help functionality in 2.x |
|
I'd like to push a few more changes in this direction to make the code more robust but wanted to get some early feedback if we are all on board with this direction. Will try to add a screenshot of the default plugins. The default is likely going to be too lengthy so unlikely to fit one screen. |
Before
After
|
|
|
| # PluginManager also has listeners for "help" | ||
| for matcher, functions in self.plug_manager.message_listeners.items(): | ||
| self.message_listeners[matcher].extend(functions) |
There was a problem hiding this comment.
I know this is how I already did it, but since any message listener needs to be defined on a Plugin and you've now created a nice place to handle all Plugin logic, maybe it makes more sense not to have a self.message_listeners on the EventHandler at all? We can simply forward any message to the PluginManager and it can then take the appropriate action itself.
There was a problem hiding this comment.
I tried implementing this on my first try but there was some code that needed to access message_listeners when only EventHandler was available. Need to give it another try now that I understand the code better.
| if help_trigger: | ||
| self.help = listen_to("^help$", needs_mention=True)(Plugin.help) | ||
| if help_trigger_bang: | ||
| if not help_trigger: | ||
| self.help = listen_to("^!help$")(Plugin.help) | ||
| else: | ||
| self.help = listen_to("^!help$")(self.help) |
There was a problem hiding this comment.
| if help_trigger: | |
| self.help = listen_to("^help$", needs_mention=True)(Plugin.help) | |
| if help_trigger_bang: | |
| if not help_trigger: | |
| self.help = listen_to("^!help$")(Plugin.help) | |
| else: | |
| self.help = listen_to("^!help$")(self.help) | |
| help_func = Plugin.help if (help_trigger_bang and help_trigger) else self.help | |
| self.help = listen_to("^!help$", needs_mention=help_trigger)(help_func) |
I have no idea what the distinction means here; just simplifying the logic
There was a problem hiding this comment.
Your suggestion doesn't implement the same logic as mine but this code needs a better solution either way.
These conditionals are mostly there to allow the user to not register help or !help for every plugin. Doing it for every plugin makes the help output unnecessarily verbose.
For backwards compatibility I kept both behind independent boolean flags.
| async def call_function( | ||
| self, | ||
| function: Function, | ||
| event: EventWrapper, | ||
| groups: Optional[Sequence[str]] = [], | ||
| ): | ||
| if function.is_coroutine: | ||
| await function(event, *groups) # type:ignore | ||
| else: | ||
| # By default, we use the global threadpool of the driver, but we could use | ||
| # a plugin-specific thread or process pool if we wanted. | ||
| self.driver.threadpool.add_task(function, event, *groups) | ||
|
|
There was a problem hiding this comment.
Can you explain this? This already had access to the driver, so what problem does moving it out of the class solve?
There was a problem hiding this comment.
Code duplication mostly. At the moment, this code needs to be used in Plugin and PluginManager due to how call_function is triggered in the driver. If we move the listeners out of the event handler class and make PluginManager less transparent this may no longer be necessary. I didn't get that far.
| # This code is a bit hairy because the function signature of an | ||
| # instance is (message) not (self, message) causing failures later | ||
| if help_trigger: | ||
| self.help = listen_to("^help$", needs_mention=True)(Plugin.help) | ||
| if help_trigger_bang: | ||
| if not help_trigger: | ||
| self.help = listen_to("^!help$")(Plugin.help) | ||
| else: | ||
| self.help = listen_to("^!help$")(self.help) | ||
|
|
There was a problem hiding this comment.
Why is this code identical to the code in Plugin? Why does it even need these arguments?
It added too much complexity for little benefit
unode
force-pushed
the
plugin-manager
branch
2 times, most recently
from
3d55d58
to
00c9acd
Compare
|
Clarifying a little more about the However since both listeners were registered for every plugin, having multiple plugins meant that each had its own set of listeners and each would respond independently, sending a rather large wall of text to the user. My solution, while trying to keep backwards compatibility, was to introduce two booleans If we are ok with breaking backwards compatibility, I would remove the |
Fixes #165 and some of the issues discussed in Discord.
Highlights:
annotations={}in@listen_to- examples and better documentation to be addedmmpy_bot.plugins.base.PluginManagerto allow customizing help messages across all enabled plugins.driver.direct_messageanddriver.create_post(..., direct=bool)to simplify sending direct messages.PluginManager.get_helpas alternative toPluginManager.get_help_string. This interface should probably be delegated to all classes that implementget_help_stringto avoid crossing interface boundaries inPluginManager.get_helpbut I didn't want to introduce more changes in this PR.Changes:
@bot helpand!helpare no longer registered by default for every enabled plugin. Instead only@bot helpis enabled and provided byPluginManager. Old behavior still available withPlugin(help_trigger=True, help_trigger_bang=True, direct_help=False), options also available toPluginManager().@bot helpdoesn't currently make use of help text generated by click. I found this output to be overwhelming and very noisy, particularly when many commands or plugins are enabled.Apologies for the noisy PR but when touching the docs where were several issues that made the linters unhappy.