DisnakeDev
feat: More verbose sync warnings
Summary
The current sync warnings returned by Discord are confusing to debug since they just reference commands by the order number they were sent in. Using some regex and string replacement we can fill in the command names, options, etc.
Checklist
- [X] If code changes were made, then they have been tested
- [ ] I have updated the documentation to reflect the changes
- [X] I have formatted the code properly by running
task lint - [X] I have type-checked the code by running
task pyright
- [ ] This PR fixes an issue
- [X] This PR adds something new (e.g. new method or parameters)
- [ ] This PR is a breaking change (e.g. methods or parameters removed/renamed)
- [ ] This PR is not a code change (e.g. documentation, README, ...)
This is a good idea, however, I don't like that the solution depends entirely on the API message. Maybe a better implementation would be to check everything on our end, but I'm not sure. Waiting for more opinions from other people
untested, like the idea
We should catch any parsing errors and silently fall back to the raw error message if anything happens while parsing.
Agreed, will add that for sure
Also, here is an example of what it looks like
SyncWarning: Failed to overwrite commands in <Guild id=931246920989544508> due to Invalid Form Body
In clearqueue.description: Must be between 1 and 100 in length.
In load.options.config.description: Must be between 1 and 100 in length.
In endqueue.description: Must be between 1 and 100 in length.
In leaderboard.options.type.choices.Points.value: Option value exceeds maximum size (100)
untested, like the idea
We should catch any parsing errors and silently fall back to the raw error message if anything happens while parsing.
Well, my point was, once the api message changes (for whatever reason) this implementation of detailed messages will stop working until we release a fix.
Instead of string operations and regex matching, it would be easier (and probably more reliable) to make use of the structured error data received from the API, which is documented.
Currently, that data is turned into the string error message by _flatten_error_dict.
Example Error Data
HTTP/1.1 400 Bad Request
Date: Wed, 04 Jan 2023 18:22:46 GMT
Content-Type: application/json
...
{
"code": 50035,
"errors": {
"17": {
"description": {
"_errors": [
{
"code": "BASE_TYPE_BAD_LENGTH",
"message": "Must be between 1 and 100 in length."
}
]
},
"nsfw": {
"_errors": [
{
"code": "BOOLEAN_TYPE_CONVERT",
"message": "Must be either true or false."
}
]
}
},
"18": {
"options": {
"0": {
"description": {
"_errors": [
{
"code": "BASE_TYPE_BAD_LENGTH",
"message": "Must be between 1 and 100 in length."
}
]
}
}
}
}
},
"message": "Invalid Form Body"
}
One possible approach I could see working would be to:
- Store
errorsas an attribute inHTTPException.__init__ - In the command sync exception handlers, check if there's an
HTTPExceptionwith code 400 - If yes, construct a better error message through the use of the list of commands being sent, the stored
errorsand calling_flatten_error_dictfor the nested errors where necessary
Instead of string operations and regex matching, it would be easier (and probably more reliable) to make use of the structured error data received from the API, which is documented. Currently, that data is turned into the string error message by
_flatten_error_dict.Example Error Data One possible approach I could see working would be to:
- Store
errorsas an attribute inHTTPException.__init__- In the command sync exception handlers, check if there's an
HTTPExceptionwith code 400- If yes, construct a better error message through the use of the list of commands being sent, the stored
errorsand calling_flatten_error_dictfor the nested errors where necessary
Sorry for what I have created, its quite ugly right now, although it doesn't need string manipulation and seems to work:
SyncWarning: Failed to overwrite commands in <Guild id=931246920989544508> due to
In clearqueue:
Description: Must be between 1 and 100 in length.
In load:
config (Option 0):
Description: Must be between 1 and 100 in length.
In mmr:
change (Option 0):
allow_disable (Option 2):
allow_disable (Option 0):
Description: Must be between 1 and 100 in length.
In leaderboard:
type (Option 1):
WinrateAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA (Choice 5):
name: Must be between 1 and 100 in length.
StreakAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA (Choice 6):
name: Must be between 1 and 100 in length.
queue_name (Option 2):
Description: Must be between 1 and 100 in length.
Thank you for the rewrite! That seems easier to understand/maintain :> I've left some comments that should fix the remaining pyright issues and hopefully improve safety a bit.
In the end, I'd really like this to be as reliable as possible, with little to no assumptions about the structure or values of the API error response. Ideally both the
int(index)casts as well as the actualoptions[index]accesses should be checked, just in case there happens to be a string key or out-of-bounds index.
Do we really need the index checks in this case? We assume the index should be valid since the error is from the passed in commands, but if we hit the IndexError, it will just default to the old error https://github.com/DisnakeDev/disnake/pull/891/files#diff-987d1693aba1ba6d912bf4c53bf050d5081c676f195961f1765a62ae4668b1a4R170
Do we really need the index checks in this case? We assume the index should be valid since the error is from the passed in commands, but if we hit the IndexError, it will just default to the old error https://github.com/DisnakeDev/disnake/pull/891/files#diff-987d1693aba1ba6d912bf4c53bf050d5081c676f195961f1765a62ae4668b1a4R170
The fallback error handler would catch that, true. I suppose we can safely assume that if a key is numeric, it's also a valid index into the commands/options/choices list.
If it's not numeric, then it should be "_errors"; this can happen e.g. at the top-level when sending a command that exceeds the size limit:
{
"code": 50035,
"errors": {
"_errors": [
{
"code": "APPLICATION_COMMAND_TOO_LARGE",
"message": "Command exceeds maximum size (4000)"
}
]
},
"message": "Invalid Form Body"
}
This currently results in the fallback error handler being used due to:
Traceback (most recent call last):
File "disnake/ext/commands/interaction_bot_base.py", line 153, in _parse_sync_warning
command = commands[int(command_num)]
ValueError: invalid literal for int() with base 10: '_errors'
This case is already handled explicitly for options, as far as I can tell, but not for commands - do we want to do something about that, or leave it?
Back from the dead, forgot about this completely sorry!
I wanted to update it so that sub commands are easier to view and don't appear as simple options. Here is a new example!
SyncWarning: Failed to overwrite commands in <Guild id=755868545279328417> due to
In /largenumber:
description:
Must be 100 or fewer in length.
In /largenumber blahblah:
description:
Must be between 1 and 100 in length.
In /largenumber heyhey:
description:
Must be between 1 and 100 in length.
options:
choice (Option 0):
description:
Must be between 1 and 100 in length.
In /leaderboard:
options:
type (Option 1):
choices:
TOOLONGTOOLONGTOOLONGTOOLONGTOOLONGTOOLONGTOOLONGTOOLONGTOOLONGTOOLONGTOOLONGTOOLONGTOOLONGTOOLONGTOOLONG (Choice 1):
name:
Must be between 1 and 100 in length.
PointsPointsPointsPointsPointsPointsPointsPointsPointsPointsPointsPointsPointsPointsPointsPointsPointsPointsPointsPoints (Choice 2):
name:
Must be between 1 and 100 in length.
GamesGamesGamesGamesGamesGamesGamesGamesGamesGamesGamesGamesGamesGamesGamesGamesGamesGamesGamesGamesGames (Choice 4):
name:
Must be between 1 and 100 in length.
queue_name (Option 2):
description:
Must be between 1 and 100 in length.
In /command foo b:
options:
my_param (Option 0):
choices:
{'A': 'a'} (Choice 0):
value:
Could not interpret "{'A': 'a'}" as string.
Still need to do more testing