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

discord: Added SelectedValue fields for Channel, Mentionable, User and Role SelectComponents #455

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

Conversation

CosmicPredator
Copy link

@CosmicPredator CosmicPredator commented Oct 24, 2024

This PR adds the following fields to ChannelSelectComponent, MentionableSelectComponent and RoleSelectComponent,

  • SelectedRoles for RoleSelectComponent.
  • SelectedChannels for ChannelSelectComponent.
  • SelectedMentions for MentionableSelectComponent.
  • SelectedUsers for UserSelectComponent.

All the fields will take in the slice of ID's of their kind and populates the SelectComponent beforehand.

Note

SelectedMentions will take in the slice of interface{} and later type-asserted to respective UserID / RoleID.
If wanted to change this implementation, please lmk.

@CosmicPredator CosmicPredator changed the title discord: Added SelectedValue fields for Channel, Mentionable and Role SelectComponents discord: Added SelectedValue fields for Channel, Mentionable, User and Role SelectComponents Oct 24, 2024
discord/component.go Outdated Show resolved Hide resolved
discord/component.go Outdated Show resolved Hide resolved
discord/component.go Outdated Show resolved Hide resolved
@@ -822,6 +858,9 @@ type MentionableSelectComponent struct {
ValueLimits [2]int `json:"-"`
// Disabled disables the select if true.
Disabled bool `json:"disabled,omitempty"`
// SelectedMentions is the slice of discord.UserID's and discord.RoleID's
// that are marked as selected by default
SelectedMentions []interface{} `json:"-"`
Copy link
Owner

Choose a reason for hiding this comment

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

If we're using a hidden field, then it's best to just have separate hidden DefaultUsers and DefaultRoles fields, then combine them in MarshalJSON.

Choose a reason for hiding this comment

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

This breaks proper order. There might be {(user, 2), (role, 3), (user, 1)} sequence.

Copy link
Author

@CosmicPredator CosmicPredator Oct 25, 2024

Choose a reason for hiding this comment

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

I agree with @MCausc78. What's wrong with using plain interfaces? It's already documented above the field for the users.

I can add an example snippet in the comments, If it's not clear enough

Copy link
Owner

Choose a reason for hiding this comment

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

What's wrong with using plain interfaces? It's already documented above the field for the users.

It's not type-safe.

The only way I would agree to moving forward with this solution is to make custom SelectedUserID and SelectedRoleID types and a SelectableID union type to combine the two. Arikawa tries its best to not have any ambiguous/error-prone any types in its code, so this shouldn't be an exception.

Copy link
Author

@CosmicPredator CosmicPredator Oct 25, 2024

Choose a reason for hiding this comment

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

@diamondburned How about this implementation?

type DefaultMention struct {
	userId *UserID `json:"-"`
	roleId *RoleID `json:"-"`
}

// Creates new DefaultMention type with only UserID
func UserMention (userId UserID) DefaultMention {
	return DefaultMention{userId: &userId}
}

// Creates new DefaultMention type with only RoleID
func RoleMention(roleId RoleID) DefaultMention {
	return DefaultMention{roleId: &roleId}
}

and when implementing,

DefaultMentions: []discord.DefaultMention{ discord.UserMention(793688107077468171), discord.RoleMention(1299037008332849212) },

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants