-
-
Notifications
You must be signed in to change notification settings - Fork 118
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
types: make application command option union easier to use #250
types: make application command option union easier to use #250
Conversation
I don't understand how this fixes anything. If anything, you should've added |
That won't work because of the way discriminated unions work. In our case we use Because there's two interfaces with the same |
No it won't? Playground Link Are you sure this is breaking anything? Could you build a playground url where it breaks? Even then, I am not merging this PR in its current state as you removed min and max_values from number options. |
Here's a playground that shows what I'm talking about: |
Easy fix. Playground Link |
🤦♂️ I'm dumb |
Are you sure that fixes it? because it also looks wrong |
When in doubt, add tests, lots of type tests. That should tell us whether or not they're fixed. Although... yeah. We first need a unit test suite in this repository. |
How do you unit test... types? Also, we do have tests, with tsd, in the tests folder |
Oh, I didn't see that directory.
Just like how we have been unit testing types in the main library for months? Even this project's tests test types: discord-api-types/tests/v9/interactions.test-d.ts Lines 26 to 27 in c7efcd5
|
Please describe the changes this PR makes and why it should be merged:
The previous way that autocomplete was enforced create types that were hard/near impossible to use. This PR sacrifices the type strength for usability making it waaay easier to adopt. So essentially
choices
andautocomplete
are no longer mutually exclusive.