sir-lancebot icon indicating copy to clipboard operation
sir-lancebot copied to clipboard

Use botcore command error manager

Open shtlrs opened this issue 1 year ago • 3 comments

This deflates the CommandErrorHandler from all the conditionals in place when it comes to error types. Most importantly, it allows us to centralize error handling of both text and slash commands in one place, and reuse any necessary logic.

shtlrs avatar Feb 18 '24 20:02 shtlrs

I'll squash the implementation commits along with the fixup one once the PR is ready to be merged.

shtlrs avatar Mar 20 '24 18:03 shtlrs

Honestly, I don't understand the benefit of this error dispatching system. I feel like this introduces extra complexity and makes the logic much harder to follow and understand.

Before, following the logic was as simple as:

  1. on_command_error is called
  2. isinstance is used to dispatch the error to the necessary handling logic

Now, to follow the logic it's:

  1. on_command_error is called

  2. It gets self.bot.command_error_manager

    • ...which is set by register_command_error_manager in bot-core
    • ...which is called by the return value of bootstrap_command_error_manager in bot/__main__.py
    • ...which is defined in bot/command_error_handlers/__init__.py, and constructs the CommandErrorManager
  3. It calls handle_error on self.bot.command_error_manager.

    • This runs through self._handlers (which was set by calls to register_handler back in bot/command_error_handlers/__init__.py) and calls should_handle_error on each handler
    • This actual should_handle_error function is split over a lot of files.
    • isinstance is used to determine whether the function should be run.
    • _get_callback is called and the correct function is selected
    • The function is called.

I'm not sure what benefit these extra steps have given us over the current code.

wookie184 avatar Mar 25 '24 18:03 wookie184

The idea initially came to make the exception handling logic reusable for both prefix command & slash command errors.

For context, we have some prefix/text commands that also have their slash version sibling, and these commands use the same logic/throw the same errors.

The question was (and I'd be interested to have your point of view), how do we reuse all the logic that existed in the error handling cog in case we have errors that came from these slash commands ?

Discord py dispatches these prefix commands through the on_command_error callback, and the ones coming from slash commands through the CommandTree's on_error callback, which both have different signatures since one relies on Context and the other relies on Interaction. So trying to relay errors from the on_error to the on_command_error one needs some sort of conversion.

I just thought of unifying both under the same interface and hiding the conversion details for the same type of error.

I understand your point of view and agree that it's slightly more complex than the setup we currently have but I don't necessarily agree that it's makes it much harder to follow nor that complex, but that could just be the fact that I'm biased since I'm the one who wrote, which makes the understanding easier.

shtlrs avatar Mar 26 '24 13:03 shtlrs