fix: options initializing with null input type#2464
fix: options initializing with null input type#2464Dorukyum merged 6 commits intoPycord-Development:masterfrom nexy7574:master
Conversation
|
TIL |
|
Is it possible we could fix the underlying cause instead? pycord/discord/commands/options.py Lines 202 to 204 in abf2eeb 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. |
|
Okay so I just spent ages walking through
|
|
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. |
Aside from a general tidy up, what sort of changes does it really need? |
A very thorough tidy up. |
Signed-off-by: Dorukyum <53639936+Dorukyum@users.noreply.github.com>
|
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? |
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. |
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 Although, if the consensus is a string default, I can make that change and update this PR 👍 |
Dorukyum
left a comment
There was a problem hiding this comment.
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 👍 |
Signed-off-by: Dorukyum <53639936+Dorukyum@users.noreply.github.com>
* Prevent Option.__init__() completing with a null input type * Update changelog (for the checkbox) * style(pre-commit): auto fixes from pre-commit.com hooks * Update CHANGELOG.md Signed-off-by: Dorukyum <53639936+Dorukyum@users.noreply.github.com> * style(pre-commit): auto fixes from pre-commit.com hooks --------- Signed-off-by: Dorukyum <53639936+Dorukyum@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Dorukyum <53639936+Dorukyum@users.noreply.github.com>
Summary
Previously, if for some reason an Option was created with a None
input_typevalue, an error would not be raised untilon_connectcalledsync_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
examples, ...).
Checklist
type: ignorecomments were used, a comment is also left explaining why.