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

Attachment type #1358

Open
wants to merge 25 commits into
base: dev
Choose a base branch
from
Open

Conversation

coolcalcacol
Copy link
Contributor

No description provided.

@coolcalcacol coolcalcacol changed the title Add "ATTACHMENT" type to Application Command Option Type, and update types Attachment type Apr 12, 2022
@Linker-123
Copy link
Contributor

Is this solely for the attachment type? There is also an attachments map in command interaction.

@coolcalcacol
Copy link
Contributor Author

@Linker-123 Is that better?

@Linker-123
Copy link
Contributor

Add types for attachment class

index.d.ts Outdated Show resolved Hide resolved
lib/structures/Attachment.js Outdated Show resolved Hide resolved
lib/structures/Attachment.js Outdated Show resolved Hide resolved
Co-authored-by: Donovan Daniels <hewwo@yiff.rocks>
lib/structures/Attachment.js Outdated Show resolved Hide resolved
lib/structures/Attachment.js Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
@coolcalcacol
Copy link
Contributor Author

what was the conclusion with the attachments class vs just using objects?

@coolcalcacol coolcalcacol requested a review from bsian03 May 20, 2022 13:04
Copy link
Contributor

@DonovanDMC DonovanDMC left a comment

Choose a reason for hiding this comment

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

sort properties alphabetically

@abalabahaha abalabahaha added this to the 0.17.x milestone Jun 29, 2022
Copy link
Contributor

@DonovanDMC DonovanDMC left a comment

Choose a reason for hiding this comment

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

Why do we have both an interface and a class named Attachment? These will be combined when using Attachment, as can be seen here

image

@coolcalcacol
Copy link
Contributor Author

The interface pre-dates this pr. Before I headed away I was considering merging it but that would mean processing the message to create attachment classes for each attachment

@DonovanDMC
Copy link
Contributor

DonovanDMC commented Jun 30, 2022

That sounds like the best idea
we've got it now, so why not use it

it's either that or rename one of them

lib/structures/Message.js Outdated Show resolved Hide resolved
@DonovanDMC DonovanDMC self-assigned this Aug 20, 2022
@DonovanDMC DonovanDMC removed their assignment Aug 29, 2022
Copy link
Collaborator

@bsian03 bsian03 left a comment

Choose a reason for hiding this comment

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

URL is used everywhere else, not Url

index.d.ts Outdated Show resolved Hide resolved
lib/structures/Attachment.js Outdated Show resolved Hide resolved
lib/structures/Attachment.js Outdated Show resolved Hide resolved
Co-authored-by: bsian03 <chharry321@gmail.com>
@bsian03 bsian03 marked this pull request as draft January 9, 2024 21:57
@bsian03 bsian03 mentioned this pull request Jan 16, 2024
21 tasks
# Conflicts:
#	esm.mjs
#	index.d.ts
#	index.js
#	lib/structures/AutocompleteInteraction.js
#	lib/structures/CommandInteraction.js
#	lib/structures/ComponentInteraction.js
@coolcalcacol coolcalcacol marked this pull request as ready for review January 19, 2024 22:01
* @prop {String?} guildID The ID of the guild in which the interaction was created
* @prop {Member?} member The member who triggered the interaction (This is only sent when the interaction is invoked within a guild)
* @prop {User?} user The user who triggered the interaction (This is only sent when the interaction is invoked within a dm)
*/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was this sorted in a recent PR and I've unsorted it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes

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

Successfully merging this pull request may close these issues.

None yet

7 participants