[commands] Handle BadUnionArgument

[laggron42]

Type

• [x] Bugfix
• [x] 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

[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.

[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?