Closed nexy7574 closed 2 months ago
TIL Fix
!= fix
:^)
Is it possible we could fix the underlying cause instead? input_type
should not be None
unless the user set it to None
I believe it may have to do with this method:
https://github.com/Pycord-Development/pycord/blob/abf2eeb0e1e78e2272e572b44f99a8de0c967989/discord/commands/options.py#L202-L204
It seems to be possible for it to return None
, perhaps we should default it to a string type instead?
@plun1331 I was unsure what the root cause is and figured just patching it like this would suffice until the proper solution can actually be found. I can certainly play around and test a few things though. If I fix the issue I'll update this PR
Okay so I just spent ages walking through Option.__init__
manually, and have found the issue. The processing is as follows:
discord.ApplicationContext
or bridge.BridgeContext
type annotations. (or, in my case, the first argument was unannotated, but assumed by the library to be context, and then my second argument was actually supposed to be context and annotated as so. If I had not annotated the second argument, it would've defaulted to a string)self
). As such, any arguments after are treated as options.input_type_is_class
is evaluated to True
, the type is not an instance of Enum
or DiscordEnum
. So we continue.from_datatype
, its observable that datatype is not a tuple, Member/User/nChannel/Role/Attachment/Mentionable or a standard datatype, skipping over the majority of the function.TypeError
is raised.None
as there is no explicit return, nor an else
for the skipped if
.input_type
is now None.__init__
, meaning the class successfully initialises with a NoneType input_type, as the rest of the references to it are merely equality comparisons, and you can compare None to anything.Now that we know what is actually causing the error, I believe an error message is more appropriate than simply defaulting to a type of string or whatever, as clearly the developer is expecting some form of context - raising an error would very easily indicate that is not what they would receive.
I'd take this as a temporary fix, although we are aware that the option handling system still requires heavy changes.
although we are aware that the option handling system still requires heavy changes
Aside from a general tidy up, what sort of changes does it really need?
Aside from a general tidy up, what sort of changes does it really need?
A very thorough tidy up.
The docs links CI keeps failing, I don't think that's my fault is it? There's so many CI jobs to look through
Any movement on this?
The docs links CI keeps failing, I don't think that's my fault is it? There's so many CI jobs to look through
CI looks good on my end, everything passed
CI looks good on my end, everything passed
Turns out I was misreading the notification github was giving me, it was giving me an error on my fork itself, not the PR. All is good here.
Upon further thought, I think we should try to solve the root issue instead. @plun1331's idea of defaulting the option type to string seems like a good solution.
Upon further thought, I think we should try to solve the root issue instead. @plun1331's idea of defaulting the option type to string seems like a good solution.
I do still disagree with this as the user would be expecting a different type, which if then used in the code would raise an AttributeError (for example, trying .defer()
) - I still think preventing the option from initialising at all would be significantly easier and more helpful to debug (since the error is obviously coming from parameters provided, not a bug in the library giving "unexpected" inputs), and prevents unexpected behaviour.
Although, if the consensus is a string default, I can make that change and update this PR :+1:
I just realised what the actual issue is. This happens when you add an argument before ctx when you shouldn't, therefore the program should throw an error.
I investigated the entire logic flow, hence why I'm adamant it should be an error, not a default :+1:
Summary
Previously, if for some reason an Option was created with a None
input_type
value, an error would not be raised untilon_connect
calledsync_commands()
. You can see the trouble I had to go through troubleshooting the vague error message here.This two-line modification will simply raise an error if input_type is None after Option() has finished initialising. If this is not a suitable fix, I am open to suggestions to make further modifications.
Information
Checklist
type: ignore
comments were used, a comment is also left explaining why.