Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add number to ApplicationCommandOption #1450

Merged
merged 3 commits into from Aug 2, 2021
Merged

Add number to ApplicationCommandOption #1450

merged 3 commits into from Aug 2, 2021

Conversation

Splingush
Copy link
Contributor

@Splingush Splingush commented Jul 28, 2021

Hello!

This commit adds NUMBER to the ApplicationCommandOption.
It also adds a method to the builder for adding it as a choice.

@arqunis arqunis added discord feature Related to Discord's functionality. enhancement An improvement to Serenity. model Related to the `model` module. labels Jul 28, 2021
@Splingush
Copy link
Contributor Author

Splingush commented Jul 28, 2021

Any double between -2^53 and 2^53

Not sure if one should enforce the range in the library, when adding options.

It does cause an issue when a choice of a larger number, e.g. 2.0f64.powi(53) + 1111., is being added. Discord accepts the slash-command to be created, but afterwards when the interaction is received it will throw an error here:

'called Option::unwrap() on a None value'

Discord handles larger doubles in their frontend, so receiving it from user-input is not an issue.

I could change the add_number_choice to include a check. It's an additional method, would returning a Result be fine? It will be different from the other add_choice-methods then.
A newtype that wraps double that can only be in this range could be another possibility.

@HarmoGlace
Copy link
Contributor

HarmoGlace commented Jul 28, 2021

In my opinion, you should not change your actual implementation to add some checks in the builder. It would be inconsistent, as for instance you can provide strings which are considered as invalid by discord but not by the library.

However you can also choose to make your own type, even if I think that it wouldn't be very useful.

@Splingush
Copy link
Contributor Author

Splingush commented Jul 28, 2021

Yeah, the add_int_choice has a similar issue. It does not let you utilize the full range of (-2^53; 2^53), because it takes an i32 (receiving it uses i64).

I think with the strings you would get an api-error (invalid-form) or it fixes it silently (I think with emoji-names?), but with the number here it fails silently and can cause an issue later on.

I would be all for enforcing it in the library, but I'm not sure if it's feasible with keeping it up to date. For example, the embed-description-limit was raised from 2048 to 4096 recently, which is only noted in the docs (which needs updating as well.)

I can just add a note about the current limit to the method, if you want.

@HarmoGlace
Copy link
Contributor

Well it is a discord issue if their API accepts an invalid number, it should be reported. A silent fail isn't good I agree

@arqunis arqunis merged commit 312ae16 into serenity-rs:current Aug 2, 2021
@Splingush Splingush deleted the application-command-option-number branch August 2, 2021 14:29
@Splingush
Copy link
Contributor Author

Hey, sorry for reopening, but I looked a bit more into it, and Discord is returning a string instead of a number when the choice-value is too large. So it could possibly be handled. But as it's outside of the documented supported range, it's undefined behaviour.

Funnily enough, if I create this command with the too-large number-choice in a different library (discord.js), I get an error when selecting the choice in the Discord-UI, so a user-facing error:

Application command interaction option values must be of type string, integer or boolean

I don't know why this is different, but that's even worse.


Well it is a discord issue if their API accepts an invalid number, it should be reported. A silent fail isn't good I agree

I think it was already discussed in the discord-api-docs-repo in several issues, so I don't think there is much use in asking them to reject these inputs.

Should I add handling for the string-case? Or is it fine to keep it the way it is? If the latter, please close again, and thanks for the guidance 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discord feature Related to Discord's functionality. enhancement An improvement to Serenity. model Related to the `model` module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants