Use botcore command error manager

[shtlrs]

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]

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

[wookie184]

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.

[shtlrs]

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.

[MarkKoz]

A few preliminary comments - still need to do a deeper review.

[MarkKoz]

The default value of a function is None. If a function can return, but has no explicit return value, then this annotation should be None.

NoReturn means it does not return i.e. control flow of the program is interrupted before it returns. I've only seen this used for functions that always throw an exception, like an abstract method that raises NotImplemented. Also, this was superceded (?) in 3.11 by Never.

[MarkKoz]

Thoughts about using @typing.override for these? It makes it more explicit that this is an override, and it helps type checkers. But if we don't use a type checker then it could be argued that this would just clutter the code.

[shtlrs]

Closing this as we haven't reached a consensus within the core dev team.