fix: options initializing with null input type

[nexy7574]

Summary

Previously, if for some reason an Option was created with a None input_type value, an error would not be raised until on_connect called sync_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

• This PR fixes an issue.
• 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, typehinting,
  examples, ...).

Checklist

• I have searched the open pull requests for duplicates.
• If code changes were made then they have been tested.
  • I have updated the documentation to reflect the changes.
• If type: ignore comments were used, a comment is also left explaining why.
• I have updated the changelog to include these changes.

[nexy7574]

TIL Fix != fix :^)

[plun1331]

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:

pycord/discord/commands/options.py

Lines 202 to 204 in abf2eeb

	input_type = SlashCommandOptionType.from_datatype(
	enum_choices[0].value.__class__
	)

It seems to be possible for it to return None, perhaps we should default it to a string type instead?

[nexy7574]

@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

[nexy7574]

Okay so I just spent ages walking through Option.__init__ manually, and have found the issue. The processing is as follows:

1. The function's argument list contains two or more 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)
2. Only the first argument is expected to be a context (excluding in classes where it is self). As such, any arguments after are treated as options.
3. The enum if block evaluates to False as, while input_type_is_class is evaluated to True, the type is not an instance of Enum or DiscordEnum. So we continue.
4. We skip past the Converter if block as nContext is not a converter subclass, leading us to the else statement.
5. The first thing that is done in that try/except/else is call from_datatype with the phony context type.
6. Inside 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.
7. We get to the final statement which purely makes sure that if the type is not one of the contexts, that TypeError is raised.
8. The function returns None as there is no explicit return, nor an else for the skipped if.
9. We then get to Option's else of the try block as there was no raised error, however input_type is now None.
10. No further processing is done in __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.

[nexy7574]

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.

[Dorukyum]

I'd take this as a temporary fix, although we are aware that the option handling system still requires heavy changes.

[nexy7574]

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?

[Dorukyum]

Aside from a general tidy up, what sort of changes does it really need?

A very thorough tidy up.

[nexy7574]

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

[nexy7574]

Any movement on this?

[plun1331]

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

[nexy7574]

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.

[Dorukyum]

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.

[nexy7574]

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 👍

[Dorukyum]

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.

[nexy7574]

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 👍