Allow modification of other modules inside modules

[squigglezworth]

I think being able to modify existing modules within a module would add a fair amount of power to the framework. For example, having a module that allows admin.py commands to be called through a channel and not just through a privmsg.

Another example would be easily adding aliases to commands, without having to do something like this # Some thoughts from @Exirel on IRC.

<Exirel> Because the main idea would be to extract command's behavior in a
         separated function.
<Exirel> But it would adds to the burden of backward compatibility, feature
         support, and other related work.
<Exirel> Like, from a design perspective, it means we would have to
         support the fact that developpers can reuse this very particular
         Sopel's code.
<Exirel> Is that a behavior we want to encourage or not?

[dgw]

Sopel modules right now are guaranteed¹ to do what their code says. No more, no less. Their behavior cannot be changed by other modules loaded into the bot, and this is A Good Thing.

Phrased as-is—"Allow modification of other modules inside modules")—I would reject this immediately. Sopel, both the bot itself and the process of writing modules for it, is supposed to be simple. Monkey-patching is not simple. We probably shouldn't encourage it.

However, I'm not going to close this as "Declined"—not yet. I want to leave the discussion open (per the IRC logs leading up to this, it was intended as a conversation starter), and keeping the issue "Open" is a nice literal counterpart to the metaphorical "open forum" this idea deserves before any decision is made.

Here are the rest of my initial thoughts on this—all heavily rewritten and edited, but still pretty rough, so you can expect logical holes and cases I didn't think of. :stuck_out_tongue:

As it stands, you can already safely import undecorated functions from other modules.

To @Exirel's point about extracting behavior into a separate function, some modules already do just that. They use decorated "wrappers" which call undecorated helper functions to do the "real" work when triggered. Even a few core modules already use this technique. It's not required, but it often makes life easier even without planning for code reuse outside of that plugin file. This requires no changes to Sopel itself, but does require that modules (including the core set) be rewritten into this pattern. I would not expect third-party modules that aren't already written this way to change.

I'm explicitly against any officially supported method of changing/removing bits of another module. The only real potential for conflicts we have now is duplicated command names or overlapping rule regex patterns, and those can't be fixed for a variety of reasons (both structural in the bot, and philosophical—i.e. no module should be able to lay exclusive claim to a trigger, whether a command name or regex pattern even if it was to become technically possible).

The pseudo-sandbox approach we have now (maybe not the right term?) is better for users, in my opinion, because they needn't worry about modules being "incompatible" with each other by way of trying to modify the same external code, or any other conflict.

My main concern (and Exirel's, I think) lies in how we would need to change the Sopel API, and/or the internal decorator implementations, to accommodate this kind of niche use-case² that would more likely let users (or third-party module developers) shoot themselves (their users) in the foot than do something cool. Making it work likely wouldn't be trivial.

---

1. Assuming all the decorators and bot internals work properly, which is why we have tests.
2. In the entire history of Willie/Sopel, as far as I know, this has only come up once: this issue. If it was widely useful, it probably (though not necessarily) would have come up before.

(Also note that I edited the OP to format the IRC quote for readability without horizontal scrolling.)

[HumorBaby]

Sopel modules right now are guaranteed to do what their code says. No more, no less. Their behavior cannot be changed by other modules loaded into the bot, and this is A Good Thing.

Additionally, no modules are currently required (files don't have to exist, let alone be enabled) for the normal function of the bot (except coretasks, which I don't consider a module in the same way as sopel/modules/*.py). IMO, this is also "A Good Thing" :grin:

---

because they needn't worry about modules being "incompatible" with each other by way of trying to modify the same external code

I feel this is the crux of why modules should not be allowed to modify one another, as I (likely mis-)understand from the OP :stuck_out_tongue:

---

They use decorated "wrappers" which call undecorated helper functions to do the "real" work when triggered. Even a few core modules already use this technique. It's not required, but it often makes life easier even without planning for code reuse outside of that plugin file. This requires no changes to Sopel itself but does require that modules (including the core set) be rewritten into this pattern

This all-but-sending (ABS; the decorated method simply "sends" the result/success/failure of the callable, with the heavy lifting done in helpers) pattern would address both possible use-cases raised in the main issue. No need to modify anything with the bot; instead, it's just something to keep in mind as modules are refactored/written, as @dgw already mentioned.

---

Now that you know how I truly feel about this issue let me propose a solution that, if this were for some reason decided to be continued with (in a fork, say), while directly addressing the need mentioned by the OP: decorate the original functions inside the sopel.module decorators with functools.wraps. (turns out this is already being done!)

If (and its a mighty big if) the user is using Python 3, they have access to a .__wrapped__ attribute which can then be used to retrieve the original method being decorated. Example:

from sopel.modules import admin
from sopel.module import commands

msg_ = admin.msg.__wrapped__

@commands('say')
def say(bot, trigger):
     return msg_(bot, trigger)

The only problem here is that it somehow removes the original require_privmsg decoration from the actual admin.msg method. So, yea, don't do this :stuck_out_tongue_closed_eyes:

[Exirel]

After further consideration, I don't think there is much to do at the moment. It's a long-term goal to make Sopel's plugins more reusable, but it's not something that is, per say, a Sopel's core feature.

It's about code quality, code re-usability. Most of our efforts today are toward better code quality overall, so the project is definitively heading in the right direction. We write more documentation, we refactor our internal interfaces (the job scheduler, the plugins loading, the setup, the logging, the CLI, the tests, and many more to come). At some point, we'll be confident enough to tell which plugins are easy to reuse, and which aren't, but I don't wish to make it a fixed goal.

So, to wrap up: I suggest to politely decline this feature request, as it is too vague to serve as a tracking issue. I believe this is already an ongoing maintenance effort without any tracking required.

[dgw]

Came across this while searching for something else, and agreed… I think we should not do this.