[commands] Handle BadUnionArgument

[laggron42]

Type

• Bugfix
• Enhancement
• New feature

Description of the changes

When using typing.Union for a command argument, multiple conversion errors can happen and discord.py manages this by raising commands.BadUnionArgument. However, Red only handles commands.BadArgument. A BadUnionArgument will currently display help.

I added the support for this by creating a new class MultipleConversionFailures that inherits from BadUnionArgument, following ConversionFailure's logic. Message formatting is half-handled by discord.py (see here) and look like this:

Could not convert "argument name" into {first converter}, {second converter} or {third converter}

A bit of code was added on our side to also display each converter error. If only one message is available, then it will be displayed alone.

Example with two displayed messages:

from typing import Union

class A(commands.Converter):
    async def convert(self, ctx, arg):
        raise commands.BadArgument("argument A failed")

class B(commands.Converter):
    async def convert(self, ctx, arg):
        raise commands.BadArgument("argument B failed")

@commands.command()
async def test(ctx, arg: Union[A, B]):
    await ctx.send("we'll never reach this point anyway")

Could not convert "arg" into A or B.
argument A failed
argument B failed

Example with a single message:

from typing import Union

class A(commands.Converter):
    async def convert(self, ctx, arg):
        raise commands.BadArgument

class B(commands.Converter):
    async def convert(self, ctx, arg):
        raise commands.BadArgument("argument B failed")

@commands.command()
async def test(ctx, arg: Union[A, B]):
    await ctx.send("we'll never reach this point anyway")

argument B failed

[mikeshardmind]

I'm not sure I'm following the rationale behind adding another exception type instead of just handling the existing one we don't handle here.

Should just handle the bad union argument.

Additionally, we really shouldn't be sending exceptions to the user without confirming the exceptions are designed as user feedback here, this makes no attempt to see what the failures are, and just joins them to send.

[laggron42]

Fixed. I didn't notice BadUnionArgument was including all command errors instead of only catching BadArguments. And I also removed MultipleConversionError, didn't realize the base class already included all needed arguments compared to ConversionError.

[mikeshardmind]

This is still presenting errors to users without any meangingful context.

Let's say for example the following converter

Union[discord.TextChannel, int]

The user will then get two messages about the same arg telling them two converters failed.

This wont be user friendly, as the number of errors they are shown won't match the actual issue.

While it's true that each of the converters displaying an error did fail, it's not intuitive to an end user to see seperate failures to one argument without an explanation.

This may require a much more in depth handling of converters if we want to be able to retain this clarity while also supporting i18n

Currently, at least when users are sent help, the help text can be tailored to explain the usage of the command and have that be translated.

[Jackenmen]

I haven't tested it but based on the previous review comments and my own expectations, I imagine that the general clarity of error messages will somewhat decrease if this were applied to Red on its own.

I think this may be an acceptable catch-all scenario but it would be great if we encouraged quality error messages by the usage of custom converters wherever possible.
I think we could provide a bunch of equivalent custom converters to commonly used Union converters. It doesn't ensure that anyone uses them but it would make for a better experience.

In addition to this, we could special-case on the Union's arguments (i.e. sth like `if converter == Union[discord.TextChannel, int]') to make sure that we don't lower the error messages in common scenarios. It shouldn't be hard to achieve and if we can convert a significant portion of error messages this way, I think it would be worth it.

Maybe we could even be a bit smarter about it than doing exact equality comparisons for the special cases but I think that this would be going into 'too much effort for not much gain' territory.

I'm not suggesting any concrete changes yet, just trying to revive discussion on this as, in general, the current UX of Union converter errors in Red leaves a lot to desire.

So, what do you think?