Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[proposal] introduce dedicated exception for bailing out of plugin functions with a user-customized error message #2656

Open
SnoopJ opened this issue Feb 9, 2025 · 3 comments

Comments

@SnoopJ
Copy link
Contributor

SnoopJ commented Feb 9, 2025

Requested Feature

I propose the addition of a new exception sopel.plugin.PluginAbort or similar that can be used by plugin authors to abort plugin event handling while also sending a fully-custom string error message via IRC.

Prompted by discussion in #sopel on Libera.chat on 9 Feb 2025, with reference to PHP's die() mechanism and sys.exit(), SystemExit in the Python stdlib. Discussion prompted by @half-duplex with participation from @dgw and myself.

Problems Solved

It is relatively common that plugin code wants to abort the handling of a command, event, etc. and it is also common that people¹ solve this problem by doing something like:

from sopel import plugin

@plugin.command("thrust")
def thrust(bot, trigger):
    if cannot_continue_predicate(trigger):
        bot.say("Error: the turbo encabulator is not available")
        return False

    operate_novertrunnion()

This leaves a pitfall where people¹ may forget to terminate control flow:

from sopel import plugin

@plugin.command("thrust")
def thrust(bot, trigger):
    if cannot_continue_predicate(trigger):
        bot.say("Error: the turbo encabulator is not available")
        # NOTE: NO RETURN! Control flow spills out of this block and will probably cause an exception.

    operate_novertrunnion()

I think it is reasonable to say that plugin authors often want to simultaneously express the semantics of "handling has stopped' and "send an error message to IRC". I propose the introduction of a dedicated exception, so that this code can be written as in the following section.

An exception would also make a plugin-level abort more convenient in cases where plugin call stacks get appreciably deep. I.e. the plugin author may detect some fatal error in helper code that does not have direct access to bot and desire a way to propagate an abort all the way to the top-level.


¹ it's me, I'm people

Alternatives

A function plugin.abort() or similar could be introduced to have the same effect. I think this may be a good idea with introduction of an exception, in the same fashion that sys.exit() is a helper that raises SystemExit for you.

@dgw has some reservations about introducing such a helper in plugin so it is possible this has some other home. I argue that plugin is a reasonable place to put it because the proposed feature is very specific to plugin author code, but I can see a valid design concern in keeping the existing suite of decorators separated from other machinery. Perhaps plugin should be a package with a plugin.error and plugin.decorators gathered into the package root to preserve existing API?

@Exirel proposed return bot.say("Custom error message") as an alternative idiom in existing API.

Notes

Implementation notes

In the bot core that handles plugin event dispatch, this feature couple be implemented by doing the moral equivalent of:

try:
    call_handler(bot, trigger)
except PluginAbort as exc:
    # more type-safety/consistency checking is probably a good idea here, contingent on API
    bot.say(exc.args)
except Exception as exc:
    existing_behavior(exc)

Example usage

import random
from sopel import plugin

BARKS = ["arf!", "grrRRR", "yip yip!"]

@plugin.command("bark")
def bark(bot, trigger):
    bot.say(random_bark())


def random_bark() -> str:
    roll = random.randint(1, 4)
    if roll == 4:
        # NOTE: this auxilliary helper does not have access to bot. This abort is not possible without plugin API redesign in the existing Sopel API
        raise plugin.PluginAbort("Cannot locate bark")
    else:
        return random.choice(BARKS)
@Exirel
Copy link
Contributor

Exirel commented Feb 22, 2025

I took some time to think about this feature. I understand the convenience of it, however I'm against it still. I've two main reasons for that:

  • it's already possible to achieve the same result with very little code
  • and I think it promotes a design pattern with more drawbacks than benefits

A one-line save?

In a previous conversation on IRC I said that I didn't see the point of something that was a one-line save or a way to prevent a PEBKAC that is so easily fixed (by a review, or by testing the error case, even manually). I argued at that time that any code added is something that will need to be maintained (however small that code may be) and adding any code must be carefully thought through. I still think that's true, and the recent question about the interaction between the new exception and the rate limit feature shows that we can't add things without a second thought because they feel convenient at time.

It is true that I'm more averse to adding new code around plugins & callables, since I've spent quite some time now removing badly designed interfaces and tools, and in general just cleaning the code.

I can understand that someone might not think it's a big deal to add a new except and move on - I tend to agree on that, it's not that much. However, I don't think that's the main reason I'm against this feature: it's the design pattern it promotes. To further explain that, I want to look at the two use cases it would fall into.

Plugin callable's level

The first use-case of this method would be at the higher level of a plugin callable, like so:

@plugin.command('example')
def example(bot, trigger):
    value = get_something()
    if not value:
        plugin.abort('No value found.')
    # ...

In theory, I don't have an issue with that use-case: it's straightforward, with a somewhat clear intent.

However in that case, the above code can be replaced with something that has the exact same intent, and is, in my opinion, as straightforward:

@plugin.command('example')
def example(bot, trigger):
    value = get_something()
    if not value:
        bot.say('No value found.')
        return
    # ...

The plugin author could, in fact, replace that with return bot.say('No value found.'), or decide to ignore rate limit in that case with a return plugin.NO_LIMIT.

In my opinion, the one-line save is not a good enough reason to add this feature.

If I want to be nitpicky, I can even argue (maybe in bad faith? don't take this argument too seriously) that this feature doesn't prevent the initial PEBCAK: the plugin author can still do bot.say instead of plugin.abort and be none the wiser. So, more code to fix a problem that isn't completely avoided?

Anywhere else

Now, the other case is the one I want to focus on: it is when the call to plugin.abort() is made at a lower level. Actually, it could be made from anywhere in the code, making it a very strong side-effect:

def get_something():
    # call me anywhere and stop the callable!
    plugin.abort('You did not see that one coming!')

@plugin.command('example')
def example(bot, trigger):
    value = get_something()
    # ...

And I think it's a bad thing. I don't want a strong side-effect, because 1. I don't want side effect at all so there is that, and 2. at least I don't want them to be able to bypass the normal flow entirely unless there is a very good reason for that - and I don't think that's the case.

Whenever I have to fix a bug, or when I write a unit tests, or when I have to refactor a feature to add something, one of the most annoying thing I have to deal with are side effects in low-level code, e.g. anything that isn't in the plugin callable's function itself.

Here are some type of side-effects I had to deal with that were quite annoying:

  • writing out to a file some information in the same function that generates said information
  • using the bot to send messages to IRC in the middle of something else, requiring to save some state all over a function instead of just dealing with the results
  • getting a value from the database in the same function where it is used (yes, I tend to split these apart)
  • altering a dict of elements based on what is already in the dict (something that was done for mode parsing, and it was a nightmare to deal with, and it had unknown bugs that were revealed only after I refactored that part)

I really don't like mixing side-effect with business logic, and I don't think it's a good idea to allow more side-effect to happen anywhere in the code for any given feature. In some case, that's simply not possible: it would be too inconvenient to prevent the plugin callable an access to the bot object with how Python and Sopel are designed.

However, in our case, the inconvenience outweighs the benefit. I want less side-effect in the code we promote, not more.

Further consideration

Now, beside the side effect, I don't think it's a good interface to raise an exception that will be handled by Sopel for that specific case: from the bot's perspective, nothing really went wrong. It's the plugin's author choice to stop here whatever the plugin is doing. It's not an error that need anything specific from Sopel - adding a log wouldn't even make sense to me. I'd argue that a plugin.error() function would make more sense, but then you already have sopel.plugins.exceptions.PluginSettingsError for example, and I don't think adding a helper for that would be that helpful.

Another thing: I would like to push asyncio more into Sopel and into plugin's callable. And by experience, I know that exceptions are kinda a bad thing with asyncio: it is more error prone, as it requires a constant attention to details, and a lot of time an exception can be silenced if not properly handled, which can make debugging very hard. So I tend to avoid exception as much as possible when there is nothing actually wrong: I need a strong reason to use an exception, not a reason not to.

In conclusion

I want to emphasis that I understand the benefit of the requested feature. I think I have a strong enough case against it that isn't just personal preferences. I think, design wise, it doesn't reflect the kind of practice I want to promote for plugin authors.

@SnoopJ
Copy link
Contributor Author

SnoopJ commented Feb 22, 2025

It's not an error that need anything specific from Sopel - adding a log wouldn't even make sense to me.

Then why are plugin exceptions routed through sopel.error?

I understand your objection, in your view the inconvenience of adding any new code outweighs the inconvenience of plugin authors having to remember the two-step dance to get this functionality. I don't agree, but I also don't think there's a lot of room for me to try and change your mind about this.

@SnoopJ
Copy link
Contributor Author

SnoopJ commented Feb 23, 2025

I was going to write up a response to this case, but discussion on IRC took a long path to "no to this feature as written, but maybe-yes to a rephrase of the feature". Mostly between @Exirel and I, but @dgw got in on the end of it and approves of the decorator rephrase in concept.

New consensus

  • the bot core delivers messages on behalf of plugins (a side effect) as a kind of a historical quirk
    • this is not considered an ideal behavior for the core by @Exirel, and I can see where he's coming from
  • it would be convenient for plugin authors to have some error-shaped affordances to them, but the side effects of these affordances (i.e. sending messages to IRC) should be owned by the plugins as much as possible.

In light of this, I agree with the rejection of adding code to sopel/bot.py for this, as I have done in my draft of #2659 which I will close.

Going forward

In the brief discussion that followed a not-brief argument to reach that consensus, we managed to agree that the plugin decorator suite could be improved by adding a mechanism to wrap plugin functions in the common side effect of sending an error message to IRC.

I realized after mulling this over for a bit that what we're basically considering here is opt-in middleware for error handling between the bot core and plugin code. If the plugin has an opinion about how to handle exceptions that occur during event handling, this is best, and we can provide a way for users to bring in error-handling behaviors for common use-cases (namely: sending a message to IRC) that are part of sopel but not part of the bot core, in the same sense that the rest of the plugin API is dominated by decorators which are more declarative on the plugin author side.

Speculative plugin code for the feature that needs more consideration, particularly in what affordances the proposed decorator might provide (fewer is likely to be better!)

import random
from sopel import plugin

# feature is opt-in, plugin functions not using it get existing behavior
@plugin.commands("whisper")
def whisper(bot, trigger):
    plugin.abort("It's too loud in here to whisper!")

# unqualified report_errors will report all exceptions as `Type: Args`
@plugin.report_errors
@plugin.commands("bark")
def bark(bot, trigger):
    plugin.abort("The dog is not yet implemented.")

# the specific exception(s) reported can be configured,
# as well as a configurable prefix instead of reporting the exception type
@plugin.report_errors(TakeError, prefix="Error: ")
@plugin.commands("take")
def take(bot, trigger):
    if random.random() < 0.5:
        plugin.abort("Random failure!")
    else:
        raise TakeError("Taking things is not yet implemented.")

With intended observable behavior (I am making up the transcript, this is not referring to real code) like:

< SnoopJ> !bark
< terribot> PluginAbort: The dog is not yet implemented.
< SnoopJ> !whisper
< terribot> Unexpected PluginAbort from SnoopJ. Message was: !whisper
< SnoopJ> !take on me
< terribot> Unexpected PluginAbort from SnoopJ. Message was: Random failure!
< SnoopJ> !take lamp
< SnoopJ> Error: Taking things is not yet implemented.

Open questions

In no particular order, some things that need more thought:

  • how much of the configurability shown above is desirable? should there be more/less? A survey of error handling in real plugin code would be a good idea
    • which customizations do plugin authors want to make to plugin-side error handling? Is calling a custom error-handling function in-bounds?
  • to what extent can we take direct inspiration from exception-handling middleware in things like Starlette? People have surely thought this problem through in other libraries with similar user-authored extension architecture

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants