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

Re-type IMessageComponentData.cs#Values as a union array of snowflakes or strings #277

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

carlst99
Copy link
Contributor

@carlst99 carlst99 commented Jan 6, 2023

Following discussion in discord/discord-api-docs#5823, my assumption in #275 appears to be incorrect, and the underlying type of the values field depends on the type of select menu used. This PR resolves that; however, given the current implementation works, I would like to leave this as a draft until the API docs PR is resolved with hopefully a final answer.

Copy link
Contributor

@MazeXP MazeXP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!
Need to be cautious with unsigned number only options that would cause a IReadOnlyList<Snowflake> even for StringSelect component.

@Nihlus
Copy link
Member

Nihlus commented Jan 10, 2023

Looks like a fine way to handle it. Please add variant test data for snowflake-typed value lists as well.

@Nihlus
Copy link
Member

Nihlus commented Jun 17, 2023

Are there any updates on this PR?

@carlst99
Copy link
Contributor Author

carlst99 commented Jan 8, 2024

Hiya @Nihlus, no sorry. Discord's response to my docs PR seemed to indicate I hadn't conveyed my request well, and owing to various things at the time I didn't respond.

I could go for round two on the Discord docs, or we could leave things as-is (deserialising to a string) given the indeterminacy of this case, or we could follow Discord's comment that the data type should always match the type of the select menu and proceed with this implementation - happy to follow your call here 🙂.

@Nihlus Nihlus added the waiting for review The PR is awaiting review by maintainers label Oct 3, 2024
@Nihlus
Copy link
Member

Nihlus commented Oct 4, 2024

This has been on the backburner for a while now - I'm happy to go with this implementation.

@Nihlus Nihlus added waiting for author The PR is awaiting an update from the author and removed waiting for review The PR is awaiting review by maintainers labels Oct 4, 2024
@carlst99
Copy link
Contributor Author

Alright. I'll double-check that this implementation is still accurate later today, and mark as ready-for-review.

@carlst99 carlst99 marked this pull request as ready for review November 13, 2024 23:28
@carlst99
Copy link
Contributor Author

Testing against Discord indicates this is still accurate 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for author The PR is awaiting an update from the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants