bot: fix per-channel configuration by plugin names #1840
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I found what I disliked here, because the code is good but I wasn't happy about it. For me the problem is that it creates a hard coupling between a Plugin and a Python module, which is exactly what I wanted to avoid with entrypoint. It looks fine for now, because a) it fixes a bug that is properly annoying and b) it works fine with the current code without having to break anything. However, it has the side effect of blocking future options. So that's a no for me. I've added more information on the bug #1839 about the context, and my suggested solution. Sorry @dgw to delay the resolution further. :( |
When this feature was first implemented, core plugins and user plugins (from `~/.sopel/modules`) both loaded in a similar way. Their Python module names matched the file names, and nobody thought to test how this feature worked (or might not) for `sopel_modules` packages. Certainly, no one thought to test entry-point plugins (new in 7.0, implemented *after* this was). To make a long story short (the full narrative is summarized in #1839), this feature was never going to work as intended in all cases. Changes to the plugin machinery simply made it *also* not work for core plugins, which made the shortcomings of its implementation much more obvious. Before passing the module contents back to `bot` during registration, the `plugins.handlers.PyModulePlugin` class (and derivatives) now adds a `plugin_name` attribute to each "relevant part" (callable). This is immediately useful for improving the per-channel filtering so it works as expected, and will likely find more use in the future (e.g. as a substitute for the long-deprecated `bot.command_groups` property). In `bot`, in addition to using the new callable attribute instead of Python's module name for per-channel filtering, I also added log output to debug mode for completeness.
dgw
force-pushed
the
fix-channel-config-plugin-names
branch
from
26c4204
to
8776354
Compare
|
@Exirel I think you'll be much happier with the replacement solution I just force-pushed. Or at least I hope you are, because the new approach is based directly on your suggestions! 😛 |
Exirel
approved these changes
Apr 9, 2020
Exirel
added a commit
to Exirel/sopel
that referenced
this pull request
May 18, 2020
The "_command_groups" attribute is used to generate the documentation for plugin's command (there is an old piece of code for some undocumented feature here but that's something else). The problem is that it tries to derive the plugin's name from the Python module the callable come from, instead of, you know, the actual plugin's name. Since we added the "plugin_name" attribute on all callable back in 8776354 (see sopel-irc#1840) we can now use it safely for the command group. One-line fix for you all!
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Annotate callables with the source plugin name during plugin registration. Use that name when applying per-channel configuration settings instead of the Python module name.
Old solution:
Translates plugin names given in the config to the module names that would/will be returned byfunc.__module__, and uses those when filtering per-channel configuration.Resolves #1839.
Checklist
make qa(runsmake qualityandmake test)make qualitylocally due to other long-term work on adding new flake8 checks that currently generate hundreds of warnings, so I'll leave this unchecked even thoughmake testdoes pass.Most of it. I made sure it works for core plugins and entry-point plugins, and the debug log output of module names I used to get this far tells me it should also work forThis disclaimer does not apply to the replacementsopel_modulesplugins and single-file plugins (a few of which I have on my test install). The only wildcard should be folder plugins.plugin_nameattribute solution.Other
The below comments are no longer relevant, but are kept for posterity.
I fully expect at least one method name to get bikeshedded. That's fine. My primary concern is making sure that the base principle of this change isn't complete lunacy. 😹I'm also fully aware that @Exirel disagrees with this approach on principle—but I'm hoping that having a PR in which to discuss it will at least push us toward either accepting this one (perhaps with modifications) or developing a different kind of fix that we can ship relatively soon.