Inline converter providing bad UX

[mysistersbrother]

This problem may be present all across the codebase, any command that relies on the Inline converter is likely susceptible to this issue. However, the problem was uncovered in tag flags command so it will be the main reference point for this issue.

Background

The .tag flags <name> [flags] allows setting or getting the flags associated with a specific tag. The two behaviours are enabled by the optional flags argument, if no flags are specified it will send the current flags for the tag, otherwise it will update the flags.

The issue was observed when specifying the flag, using Inline format.

As seen in the above image, the bot does not update the flags but instead replies with the current flags.

However, if you specify a space after the backtick, it will work as expected:

Root Cause Analysis and Fixes

To understand why this happens, we first need to understand how argument parsing works in discord.py. A brief overview for parameters can be found in the docs. Notably, arguments are split by words (which are separated by whitespace). That means an invocation such as .command arg1 arg2 will invoke the commands with two separate arguments. The library also allows for whitespace in a single argument through the use of double quotes, e.g. .command "one arg". To include quotes in an argument you need to escape them with a backslash: .command \".

Custom converters for these commands can be created which take the string argument as parsed by discord.py and convert it to the target type. Inline is one of these converters.

This information already raises interesting questions about how the Inline converter works, the flags command has many inputs that naturally include whitespace such as

.tag flags ping_smay `{"mentions": true}`

There is whitespace after the colon in the flags, so you would expect this to get fragmented into multiple arguments. One possible way this could be solved is a keyword-only argument which would cause the entire input to be consumed. This would work for the flags command, but there are other commands that take multiple arguments of Inline where this would not work.

The first logical step is to take a look at the source code for the Inline converter, specifically the convert method. We can see that it uses a regex and matches against Context.view.buffer rather than arg. Taking a look at the documentation, there is no view member in Context. This is due to view actually being an internal library member which is not meant for public use. It is a StringView which is used for parsing the input into 'words' (arguments). The buffer contains the entire input which is how the converter gets around the whitespace issue, it matches against the entire input instead of the arg provided by discord.py. The converter then manually advances the view such that further arguments will be taken from after the matched input.

The argument passed to the converter is resolved by calling get_quoted_word. This method will handle whitespace and quotes for arguments that contain them. The issue is that, naturally, the library calls it before invoking the converter. This means the converter does not know where in the buffer to start matching from because it has already been advanced past the current argument. A naive approach would be to simply look back len(arg) characters, but this would fail due to the special parsing rules:

Discord message: .command "argument one"
Argument value: argument one

Discord message .command \"
Argument value: "

The lengths of arg differs to amount of characters you should retreat by in the view. To handle this the converter used the undo_get_quoted_word method which is not provided by discord.py but instead defined within this repository. This method will handle the above cases by comparing the argument to the actual input.

This all sounds well and good, and it doesn't really explain why we are argument is not getting parsed. The type signature for the flags argument has an Optional which is correct for typing reasons, but has other implications for discord.py. The Optional type is a special converter in the library. It causes the parser to skip the parameter if parsing fails, however, this also means any error message during parsing will be hidden. By removing this type annotation we can find more information:

This exposes the issue, as explained earlier quotes are treated specially by the parser in discord.py. The quotes in question here are not escaped so the parser complains at their presence. By adding whitespace after the backtick, the parser will only process up to the backtick in get_quoted_word because it stops after hitting whitespace. Control will then be passed to the converter which will advance the entire StringView past the problematic input so the error is not hit. This also means the error can be worked around by adding a space anywhere before the first quote.

Whitespace is also the reason this issue doesn't show up for codeblocks, the newline after the first 3 backticks form a word boundary and prevent the parser from getting to the problematic input.

This problem is quite difficult to fix without more hacky code that touches the internals of the library, the proper way to handle this would be to use one keyword argument so that the library gives us one large string which we can then manually process within the command. You cannot even work around this problem by escaping the quotes because the converter does not use the argument as parsed by the library:

For the case of the flags command, a potential solution would be to convert it to a keyword-only argument as flags is the last argument so there is no problem with it consuming the rest of the input. The keyword only argument would prevent the library from parsing the input word by word and just pass whatever remains to the converter in its entirety. Unfortunately, this still fails because the undo_get_quoted_word method will over-calculate how far it needs to move back in the buffer, as it assumes the quotes in arg had to be escaped. Instead, we would need a different converter which uses the argument as given instead of trying to match against the internal buffer of the library.

Due to these reasons there is no trivial and clean fix. There is also no fix that can affect all usages in the codebase of such converters without being extremely hacky and touching more library internals. The possible fixes for the flag command would be as mentioned, new converters and a keyword-only argument or handling the parsing manually in the command.

There is also the issue of Optional concealing error messages which I have raised #102 for.