-
-
Notifications
You must be signed in to change notification settings - Fork 4k
feat: message structures #10982
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
base: main
Are you sure you want to change the base?
feat: message structures #10982
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #10982 +/- ##
==========================================
+ Coverage 45.23% 45.49% +0.25%
==========================================
Files 307 352 +45
Lines 17189 17988 +799
Branches 1747 1752 +5
==========================================
+ Hits 7776 8183 +407
- Misses 9400 9792 +392
Partials 13 13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looking pretty good, some docstring errors and a couple things to discuss so far.
Also overall discussion, should we doc every param like types and kinda mainlib do rn. So many of them are very self-explanatory (especially the substructure ones) but I think we probably should do a lot of it anyways?
export * from './TextDisplayCompoinent.js'; | ||
export * from './TextInputCompoinent.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export * from './TextDisplayCompoinent.js'; | |
export * from './TextInputCompoinent.js'; | |
export * from './TextDisplayComponent.js'; | |
export * from './TextInputComponent.js'; |
filename typos
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo in the name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo in the name
: APIBaseComponent<ComponentType>; | ||
export class Component< | ||
Type extends APIMessageComponent | APIModalComponent, | ||
Omitted extends keyof Type | '' = '', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about the base component, but at least for the rest of them we should be overriding the static DataTemplate for type purposes
I suppose you could want to not store something on any components, but not sure if the tighter type would help that.
Omitted extends keyof APIActionRowComponent<Type> | '' = '', | ||
> extends Component<ComponentDataType<ComponentType.ActionRow>, Omitted> { | ||
/** | ||
* @param data - The raw data received from the API for the connection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a connection (everywhere)
super(data as InteractionMetadataType<Type>); | ||
} | ||
|
||
public get authorizingIntegrationOwner(): Readonly<APIAuthorizingIntegrationOwnersMap> | null { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
torn on this one
/** | ||
* The user ids that participated in this call | ||
*/ | ||
public get participants(): readonly string[] | null { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets discuss
public static override DataTemplate: Partial<APIMessageMentions> = { | ||
set mentions(_: APIUser[]) {}, | ||
set mention_channels(_: APIChannelMention[]) {}, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we defaulting removing these?
* @typeParam Omitted - Specify the properties that will not be stored in the raw data field as a union, implement via `DataTemplate` | ||
* @remarks has substructures `MentionChannel`, `User` which need to be instantiated and stored by an extending class using it | ||
*/ | ||
export class MessageMentions<Omitted extends keyof APIMessageMentions | '' = ''> extends Structure< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this class exist? These all seem to be properties of message directly on discord's side. I guess we probably make everything under mentions in discord.js but I don't think we should do that here. (Also everything in this structure is an array that would probably be replaced by a cache-level accessor for the respective ids)
>; | ||
|
||
/** | ||
* Represents video data in an embed on a message. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nop (all the metadatas)
Please describe the changes this PR makes and why it should be merged:
Status and versioning classification: