Implement PluginManager (3 of 3) #226

unode · 2021-05-30T17:38:35Z

Reopened #212 targeting main instead of master.

unode · 2021-05-31T01:56:20Z

Some significant changes from #212 - Tests are not yet in sync. More on that in the next push:

Plugin.help and Plugin.get_help_string is now HelpPlugin.help and HelpPlugin.get_help_string.
The logic to collect help information about plugins is now PluginManager.get_help.
HelpPlugin is enabled by default but needs to be explicitly enabled if you specify your own plugins.
PluginManager is now available to Plugin instances as self.manager initialized via Plugin.initialize
help_trigger and help_trigger_bang are no more. There's only settings.RESPOND_CHANNEL_HELP that defaults to False and defines if the bot should respond to !help in any channel.
Arbitrary information can be passed via listen_to and listen_webhook as kwargs and made available via PluginHelpInfo.annotations. (e.g. @listen_to(..., category="webhook", syntax="Command Arg1 Arg2")
- Two attributes are currently recognized by HelpPlugin.get_help_string:
  - category - grouping functions with the same category label (see screenshot below)
  - syntax - providing an alternative to displaying the regex pattern. Instead of ^reply at (.*)$ you can get reply at TIMESTAMP or any arbitrary text.
All help information is now set via function/class docstrings instead of get_help_string. The first line of the docstring will be displayed in the help output. The entire docstring is available as PluginHelpInfo.function_docfull. Plugin level docstrings are also available, see plugin_docfull and plugin_docheader
get_help_string is no longer part of the base ABC Plugin class nor Function subclasses.
Docstrings were added to the ExamplePlugin where missing.

The current help output with plugins HelpPlugin, ExamplePlugin and WebHookExample looks like:

.pre-commit-config.yaml

unode · 2021-06-13T17:55:04Z

I think this is ready for a final of review.

unode · 2021-06-13T19:08:57Z

This PR will also close #165

attzonko · 2021-06-14T21:15:24Z

Again @unode thanks for putting in all this work. I have a quick request, can you elaborate on what you mean by the bullet below and what exactly does it mean for users who use their own plugins:

HelpPlugin is enabled by default but needs to be explicitly enabled if you specify your own plugins.

unode · 2021-06-15T12:00:37Z

Again @unode thanks for putting in all this work. I have a quick request, can you elaborate on what you mean by the bullet below and what exactly does it mean for users who use their own plugins:

HelpPlugin is enabled by default but needs to be explicitly enabled if you specify your own plugins.

I mean this

mmpy_bot/mmpy_bot/bot.py

Line 37 in a584bf2

[HelpPlugin(), ExamplePlugin(), WebHookExample()]

If you run the bot without adding any custom plugins, it will be included among the default plugins.
If add custom plugins (the most likely case) you should include it, or a subclass of it, in the list of plugins to enable.

Initially I thought of having it always enabled (i.e. plugins + [HelpPlugin()]) but that would remove the ability to define or customize your own help plugin.

jneeven

Nice, thanks for all the work 🙌🏼! I think having a separate PluginManager is great, I just have a bunch of questions and suggestions here and there. It's a huge PR, so there are loads of comments 🙃 In the future it'd be nicer to split a big one like this into maybe ~4 PRs, that makes it much easier to see what's going on. I haven't look at all the details yet, so will probably have to do another round of reviewing later.

mmpy_bot/bot.py

mmpy_bot/function.py

jneeven · 2021-06-20T13:19:12Z

mmpy_bot/function.py

        )

+        # Preserve docstring
+        new_func.__doc__ = func.__doc__


Why is this necessary? We can definitely explicitly store the __doc__ of the wrapped function somewhere, but I'd prefer not to modify the __doc__ of the wrapping Function since __doc__ is normally class-specific, not instance-specific.

This one was tricky. We now have PluginHelpInfo.function_doc[header|full] as well as PluginHelpInfo.plugin_doc[header|full] which contain the method and class level Plugin docstring, respectively.
With the way decorated functions are transformed into MessageFunction or WebHookFunction and the fact that methods can be decorated multiple times (multiple listen patterns, click decorators, ...), this was the simplest way to propagate the docstring without having to do some kind of conditional code using isinstance().

The test tests/unit_tests/plugin_manager_test.py::TestPluginManager::test_get_help verifies if the docstring is propagated correctly. Commenting these lines breaks it.

I still don't understand this. The base Function class has self.docstring = self.function.__doc__ or "", where self.function is always the absolute innermost function (i.e. if you pass it a Function, it will keep unwrapping it until it reaches the callable that this Function wraps). Why then would we need to change self.__doc__ as well, and can't we just use self.docstring?

There might be some confusion on my part. When I tried to capture this in a test, the test fails unless I included the line above.

Can you perhaps have a look at the test tests/unit_tests/plugin_manager_test.py::TestPluginManager::test_get_help and confirm that the test and its assumptions are correct?

I can't judge what's going on just from looking at the code, so will have to try running it later

mmpy_bot/plugins/base.py

tests/unit_tests/plugin_manager_test.py

See attzonko#226 (comment) (cherry picked from commit c863a9e1d03dfbe1c3d72dcd0ae6f22403e0fde6)

unode

Nice, thanks for all the work 🙌🏼! I think having a separate PluginManager is great, I just have a bunch of questions and suggestions here and there. It's a huge PR, so there are loads of comments upside_down_face In the future it'd be nicer to split a big one like this into maybe ~4 PRs, that makes it much easier to see what's going on. I haven't look at all the details yet, so will probably have to do another round of reviewing later.

Thanks a lot Jelmer. Your detailed review is super appreciated.
Indeed this grew more than expected so, take your time. I also took mine 😅 .

I tried to get different PRs at some point but wasn't clear how to go about dependencies between PRs. I can pull some things out if you think it would make your life easier.

unode · 2021-06-20T19:48:22Z

mmpy_bot/plugins/base.py

+    def __iter__(self):
+        return iter(self.plugins)
+
+    def initialize_manager(self, driver: Driver, settings: Settings):


This one I did intentionally to avoid mixing up with Plugin.initialize(). This was quite confusing when PluginManager was also behaving as a Plugin. It's no longer but I still kept it to highlight the distinction. Particularly relevant since they now receive different sets of arguments (Plugin.initialize() also receives a reference to PluginManager).

mmpy_bot/plugins/base.py

mmpy_bot/plugins/example.py

unode · 2021-06-20T20:44:06Z

tests/unit_tests/plugin_manager_test.py

+msg_listeners = {
+    re.compile("pattern"): expand_func_names(FakePlugin.my_function),
+    re.compile("direct_pattern"): expand_func_names(FakePlugin.direct_function),
+    re.compile("another_async_pattern"): expand_func_names(
+        FakePlugin.my_async_function
+    ),
+    re.compile("async_pattern"): expand_func_names(FakePlugin.my_async_function),
+    re.compile("hi_custom"): expand_func_names(FakePlugin.hi_custom),
+    # Click commands construct a regex pattern from the listen_to pattern
+    re.compile("^click_command (.*)?"): expand_func_names(FakePlugin.click_commmand),
+}


This is how it was before. That part of the code wasn't touched. See

mmpy_bot/mmpy_bot/function.py

Line 216 in 9d32fdd

reg = f"^{reg.strip('^')} (.*)?" # noqa

mmpy_bot/plugins/base.py

unode · 2021-06-20T22:22:30Z

Pulled #233 out of this one. That's a pretty self contained feature.

See attzonko#226 (comment) (cherry picked from commit c863a9e1d03dfbe1c3d72dcd0ae6f22403e0fde6)

unode · 2021-06-25T13:52:22Z

After trying to customize a little the help info I noticed the current approach actually forces you to modify PluginManager.get_help if you want to filter out some functions and HelpPlugin.get_help_string if you want to customize the format of the help.

To simplify the use-case of filtering listeners, I now introduced a HelpPlugin.get_help method. It currently only calls to PluginManager.get_help but provides a location for users to override what is included without having to redefine HelpPlugin.get_help_string entirely.
See also https://github.com/attzonko/mmpy_bot/pull/226/files#diff-6cc66ec79b3b320572cf875b2e985afab611933a46d625218fc030c3a000131dR188 as an example of such use-case.

jneeven

Round two 😅 These reviews are taking me a long time, so I would be grateful if you could either make PRs into this branch and/or not force-push to it, so I can look only at the latest changes

jneeven · 2021-07-03T12:09:52Z

mmpy_bot/function.py

-                self.docstring = self.function.get_help(ctx).replace(
-                    "\n", f"\n{spaces(8)}"
-                )
+                self.docstring += f"\n\n{self.function.get_help(ctx)}"


I think this is what causes those duplicate docstrings you showed earlier in Discord. click already includes the docstring in get_help, so this shouldn't be +=.

Suggested change

self.docstring += f"\n\n{self.function.get_help(ctx)}"

self.docstring = f"\n\n{self.function.get_help(ctx)}"

I tried = before but then we lose all of the help from click.
I'm not happy with either of the solutions but I think we'll need to address click UI/UX in another PR.
I agree that click is convenient for argument parsing but the help information is (IMO) too verbose for a chat-bot.

I tried = before but then we lose all of the help from click.

What do you mean? The "help from click" is what is returned by self.function.get_help(ctx) right?

Yes to self.function.get_help(ctx). If memory serves the issue was functions decorated with both @listen_to and @click.... The inner click decorated is processed and function.get_help(ctx) is called but as the outer decorator (listen_to) is processed later, this was overwriting what was done before, hence the +=.

mmpy_bot/function.py

jneeven · 2021-07-03T12:15:47Z

mmpy_bot/function.py

        )

+        # Preserve docstring
+        new_func.__doc__ = func.__doc__


I still don't understand this. The base Function class has self.docstring = self.function.__doc__ or "", where self.function is always the absolute innermost function (i.e. if you pass it a Function, it will keep unwrapping it until it reaches the callable that this Function wraps). Why then would we need to change self.__doc__ as well, and can't we just use self.docstring?

mmpy_bot/plugins/base.py

jneeven · 2021-07-03T12:49:52Z

tests/unit_tests/plugin_manager_test.py

+msg_listeners = {
+    re.compile("pattern"): expand_func_names(FakePlugin.my_function),
+    re.compile("direct_pattern"): expand_func_names(FakePlugin.direct_function),
+    re.compile("another_async_pattern"): expand_func_names(
+        FakePlugin.my_async_function
+    ),
+    re.compile("async_pattern"): expand_func_names(FakePlugin.my_async_function),
+    re.compile("hi_custom"): expand_func_names(FakePlugin.hi_custom),
+    # Click commands construct a regex pattern from the listen_to pattern
+    re.compile("^click_command (.*)?"): expand_func_names(FakePlugin.click_commmand),
+}


Did you accidentally link the wrong file? I don't see the relevance of that line here. I think the relevant test was this one, and it did not require duplicating the listeners like this

unode · 2021-07-05T09:25:54Z

Round two sweat_smile These reviews are taking me a long time, so I would be grateful if you could either make PRs into this branch and/or not force-push to it, so I can look only at the latest changes

Sorry, I will stop pulling things in/out until the review is complete.

jneeven · 2021-08-15T09:54:42Z

tests/unit_tests/plugin_manager_test.py

+        # Prior to initialization there is no help
+        assert self.plugin_manager.get_help() == []


This seems to rely on self.plugin_manager not having been initialized, but it's initialized first thing in test_init?

Doesn't pytest run the two tests independently? i.e.

t = TestPluginManager() t.setup_method() t.test_get_help() t = TestPluginManager() t.setup_method() t.test_get_init()

jneeven

Your recent changes look good to me and I've resolved some comments. Some comments I left earlier are still relevant though, could you have another look at those?

I think we're nearing convergence here, so I propose the following:

Let's try to first resolve all comments open in this PR. I won't add any new ones, but we can continue discussing the ones that already have some discussion going on.
Once everything is resolved, I'll need to have a fresh look at all the changes here, and since this is an enormous PR and there have been months in between each time I looked at it, it would be best to split this into several easily reviewable PRs so it becomes very clear what changes are being made in what context, and maybe @attzonko can then have a look at it as well if he wants. I think there are easily several PRs worth of changes here, such as the introduction of metadata, the introduction of the FunctionInfo, etc. If the interdependence between all these things is an issue, feel free to open one initial PR of very limited scope, and then make the others PR merge into that one rather than directly into main. We can then wait with merging that central PR until all the individual changes have been reviewed and merged into it without needing to look at 17 files at a time 😅

unode · 2021-08-16T10:28:34Z

Thanks @jneeven!

I'll give it another try at pulling out different PRs. Will likely go with your suggestion of making the PRs against the previous.

aconitumnapellus · 2021-08-30T12:03:10Z

Im eagerly awaiting this PR! Currently have a .help command to show what commands are available and what they do

unode · 2022-07-15T21:02:27Z

See #323 and #324 for an attempt to split this PR.
The current history will be rebased on the other two PRs once merged.

Allows passing custom function annotations that can be used by the help plugin to format or group functions (cherry picked from commit 66bb750)

(cherry picked from commit 8500e031ab34de5ead988e68fb41d4de6a4968a0)

Defines an interface for listeners to allow easier collection and formatting of help information

This interface splits up code from Plugin and separates the role of EventHandler from Plugins.

codeclimate · 2022-07-15T21:39:23Z

Code Climate has analyzed commit ef7180a and detected 3 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	3

View more on Code Climate.

unode marked this pull request as draft May 30, 2021 17:38

unode force-pushed the plugin-manager branch from 0f95dff to 4509092 Compare May 31, 2021 00:55

unode force-pushed the plugin-manager branch from b6fd867 to 3b3077c Compare May 31, 2021 02:08

jneeven reviewed Jun 3, 2021

View reviewed changes

.pre-commit-config.yaml Outdated Show resolved Hide resolved

unode mentioned this pull request Jun 11, 2021

Add pre-commit framework configuration and lint documentation #228

Merged

unode force-pushed the plugin-manager branch 2 times, most recently from f601607 to 7d4d2af Compare June 13, 2021 17:36

unode marked this pull request as ready for review June 13, 2021 17:55

unode force-pushed the plugin-manager branch from a4be161 to a584bf2 Compare June 13, 2021 18:26

jneeven self-requested a review June 14, 2021 07:37

jneeven reviewed Jun 20, 2021

View reviewed changes

unode added a commit to unode/mmpy_bot that referenced this pull request Jun 20, 2021

Move conditional on WEBHOOK_HOST_ENABLE outside the function

2ba67a1

See attzonko#226 (comment) (cherry picked from commit c863a9e1d03dfbe1c3d72dcd0ae6f22403e0fde6)

unode commented Jun 20, 2021

View reviewed changes

unode added a commit to unode/mmpy_bot that referenced this pull request Jun 24, 2021

Move conditional on WEBHOOK_HOST_ENABLE outside the function

07d6b18

See attzonko#226 (comment) (cherry picked from commit c863a9e1d03dfbe1c3d72dcd0ae6f22403e0fde6)

unode force-pushed the plugin-manager branch from 3ca2874 to b57bdca Compare June 25, 2021 00:35

jneeven reviewed Jul 3, 2021

View reviewed changes

jneeven reviewed Aug 15, 2021

View reviewed changes

attzonko marked this pull request as draft June 18, 2022 18:39

unode mentioned this pull request Jul 15, 2022

Implement FunctionInfo (1 of 3) #323

Merged

unode mentioned this pull request Jul 15, 2022

Implement HelpPlugin (2 of 3) #324

Closed

unode force-pushed the plugin-manager branch from 2e2d486 to 8dea93b Compare July 15, 2022 21:00

unode changed the title ~~Implement PluginManager~~ Implement PluginManager (3 of 3) Jul 15, 2022

unode added 6 commits July 15, 2022 23:15

Add wildcard metadata attribute to Function

d08117b

Allows passing custom function annotations that can be used by the help plugin to format or group functions (cherry picked from commit 66bb750)

Document extra metadata for help usage in listeners

8018720

(cherry picked from commit 8500e031ab34de5ead988e68fb41d4de6a4968a0)

Add is_click_function to Function for API consistency

aee48d3

Add FunctionInfo object

8c13a00

Defines an interface for listeners to allow easier collection and formatting of help information

Document FunctionInfo objects and how to customize help messages

b51a52a

Implement help text formatting as a plugin

763895c

unode force-pushed the plugin-manager branch from 8dea93b to 9e696a6 Compare July 15, 2022 21:30

unode added 11 commits July 15, 2022 23:38

Remove help functions superseeded by HelpPlugin

a9bdb63

Implement PluginManager interface

91c2fec

This interface splits up code from Plugin and separates the role of EventHandler from Plugins.

PluginManager and HelpPlugin should be included in __init__

5a00217

Use PluginManager in Bot class

bc6ae49

Implement PluginManager interface in EventHandler

3b111da

Fix type checking and propagation of docstrings through Function

12d4559

Simplify Plugin code by use of PluginManager

a871363

Cleanup imports

bd63ba4

Add category metadata to example plugin for HelpPlugin formatting

5e1c33e

Use PluginManager in webhook example

7dce310

Add PluginManager tests and fix existing ones

ef7180a

unode force-pushed the plugin-manager branch from 9e696a6 to ef7180a Compare July 15, 2022 21:39

This was referenced Jul 15, 2022

PluginManager interface (2 of 3) #325

Closed

Implement PluginManager and HelpPlugin #326

Merged

unode closed this Aug 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement PluginManager (3 of 3) #226

Implement PluginManager (3 of 3) #226

unode commented May 30, 2021

unode commented May 31, 2021

unode commented Jun 13, 2021

unode commented Jun 13, 2021

attzonko commented Jun 14, 2021

unode commented Jun 15, 2021 •

edited

Loading

jneeven left a comment

jneeven Jun 20, 2021

unode Jun 20, 2021 •

edited

Loading

jneeven Jul 3, 2021

unode Aug 12, 2021

jneeven Aug 15, 2021

unode left a comment

unode Jun 20, 2021

unode Jun 20, 2021

unode commented Jun 20, 2021

unode commented Jun 25, 2021

jneeven left a comment

jneeven Jul 3, 2021

unode Jul 5, 2021

jneeven Aug 15, 2021

unode Aug 16, 2021

jneeven Jul 3, 2021

jneeven Jul 3, 2021

unode commented Jul 5, 2021

jneeven Aug 15, 2021

unode Aug 16, 2021

jneeven left a comment

unode commented Aug 16, 2021

aconitumnapellus commented Aug 30, 2021

unode commented Jul 15, 2022

codeclimate bot commented Jul 15, 2022

	self.docstring += f"\n\n{self.function.get_help(ctx)}"
	self.docstring = f"\n\n{self.function.get_help(ctx)}"

		# Prior to initialization there is no help
		assert self.plugin_manager.get_help() == []

Implement PluginManager (3 of 3) #226

Implement PluginManager (3 of 3) #226

Conversation

unode commented May 30, 2021

unode commented May 31, 2021

unode commented Jun 13, 2021

unode commented Jun 13, 2021

attzonko commented Jun 14, 2021

unode commented Jun 15, 2021 • edited Loading

jneeven left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unode Jun 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unode commented Jun 20, 2021

unode commented Jun 25, 2021

jneeven left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unode commented Jul 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jneeven left a comment

Choose a reason for hiding this comment

unode commented Aug 16, 2021

aconitumnapellus commented Aug 30, 2021

unode commented Jul 15, 2022

codeclimate bot commented Jul 15, 2022

unode commented Jun 15, 2021 •

edited

Loading

unode Jun 20, 2021 •

edited

Loading