rules: refactoring of Sopel's rule system to an object-oriented approach #1873
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Exirel
force-pushed
the
core-object-oriented-rules
branch
from
c6af90f to
e747d30
Compare
This comment has been minimized.
This comment has been minimized.
|
I've just pushed a new commit that contains modification to handle the
The next step is to modify And after that, maybe in Sopel 7.2 (or before if I found the time), I'll suggest a totally new interface for documentation. But right now, I'm not sure about what we need exactly, so I stick to being backward compatible - this is already an improvement anyway! |
This comment has been minimized.
This comment has been minimized.
This is done. With added tests! |
This comment has been minimized.
This comment has been minimized.
|
@dgw fully ready for review now! |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Oh my, 104 line notes. And I haven't even tested anything; this is just a paper review. (Not that I should need to test it, because you wrote literally 2,000+ lines of tests, but y'know… 😎)
Don't panic too much, though. There are a lot of repetitions of the same typo fix (due to copy/paste or what-have-you), and not many comments that will require actual thought.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
So, I've read all your comments, fixed what I could, didn't rebase anything, and replied with what I think are relevant answers. @dgw I can see that you have question about the new interface, because you saw that it wasn't totally finished. I believe that too, and I'd like to improve it in future PR, rather than here. I prioritized backward compatibility over new feature, and I think new PRs like #1881 are the way to go. |
sopel/plugins/rules.py
Outdated
| :param str plugin: Optional filter on the plugin name | ||
| :return: ``True`` if the command exists, ``False`` otherwise | ||
|
|
||
| By default, this method doesn't search commands by their aliases. If |
There was a problem hiding this comment.
Let's look at it from the perspective of a possible use-case, then.
Let's say I'm a plugin author who wants, for some reason, to programmatically register a new command. I want to be responsible, so I don't want to register my command if its name is already bound to something. In this case, I'd want aliases to be included, so I don't accidentally register something that will also trigger another handler unintentionally.
I'm having trouble thinking of a case when I (as a plugin author) wouldn't want to know about aliases, honestly. I get that explicitly requesting it "feels better", but the default behavior should reflect what most users of the method will expect. We have no hard data, obviously, but I anticipate most users would want aliases included based on my understanding of what this method (and its friends) is for.
Co-authored-by: dgw <[email protected]>
I modified the loader.clean_callable function to stop adding regex to the `rule` attribute on a callable that doesn't need to: * commands, * action commands, * nickname commands, don't need to have a specitic "rule" attribute. Therefore, when the bot registers a callable, it can register it as an anonymous rule and as a command, as expected by plugin authors. Tests have been modified accordingly.
Co-authored-by: dgw <[email protected]>
Co-authored-by: dgw <[email protected]>
Exirel
force-pushed
the
core-object-oriented-rules
branch
from
b7e0f9f to
15173d9
Compare
|
Oh and this time Travis reported its status properly! All green! |
This comment has been minimized.
This comment has been minimized.
|
This pull request introduces 3 alerts when merging 04c3821 into 1c65ebb - view on LGTM.com new alerts:
|
Description
I replaced the function-based rule management of Sopel by a more object-oriented approach:
sopel.bot.Sopel
In this PR, the bot's dispatch and triggered rule execution are modified to use the new rule system. It delegates the callable registration to the
Manager(see below), and deal less with callables, and more with higher objects, such as "Plugin" and "Rule".I've kept the
Sopel.callmethod for now, because the URL Callbacks system still uses it. However, I plan on changing that, so that URL Callbacks will be part of the new Rule system, to be unified under the same mechanisms (and I've to resist the urge to say "rules" here because it could be confusing). It is possible to do that either in Sopel 7.1 or Sopel 7.2. My biggest concern at the moment is to see if these changes won't clash with the ongoing work in #1805 (spoiler: they probably do).sopel.plugins.rules.Manager
The
Managerclass is the entry-point to the Rule-based management system: you can register rules, commands, nick commands, and action commands. It is used bysopel.bot.Sopelto manage all of that for the bot.There are 3 key concept:
The manager will store/remove rules based on their plugin, so it's now easier to add/remove a plugin: there is no "is this the right callable?" shenanigan anymore. Beside, the regexes part is totally absent from the mix, because Sopel doesn't care for that when adding or removing a plugin. The regex is not part of a Rule's identity.
sopel.plugins.rules.AbstractRule (and subclasses)
The
AbstractRuledefines the new rule interface: what you can ask of it, how you can work with it, and how you can manipulate it. See its docstring for more information about the relevant data it contains and manage.The key point for me was to add a classmethod named
from_callable, which ensure that it's always possible to create a rule object from a plugin callable. I believe plugin author shouldn't have to care about all the fancy tricks we use internally to write a plugin. Functions are fine, decorators work great, and I don't plan on changing that anytime soon.However, I needed to switch the focus of the bot from "function" and "regexes" to a more semantic approach: rules, commands, labels, plugins. These are the key components of the system.
Also, I totally added the
sopel.module.labeldecorator, so now Rules have labels (see below for more).sopel.module.label
Commands (and nick/action commands) have a name (and aliases), so it was quite easy to disable them or enable them per-channel: all you had to do was using
.helpto know the list of commands, and you could then disable them. Easy peasy. Sadly, anoynomous rules required you to know the exact name of the function for the same purpose, which is bonkers. Also it was a pain to debug things or to read the logs.Now it's easier: rules have a label now. By default, it is derived from their handler's name (the function's
__name__attribute that is). But because it's not always what you want, you can use a new decorator for that:Then, when loading the rule (using
Rule.from_callable), doingstr(rule)will outpot something like that:In that case, the
recorderis the plugin name,logallthe rule's label, and1is the number of regexes this rule object uses.Output Example
Because I implemented the magic method
__str__for all rules, this is now what the startup logs looks like, with log-level set to DEBUG (note how easier to read it is now):Checklist
make qa(runsmake qualityandmake test)