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 4 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
@ghost
Copy link

ghost commented Apr 12, 2022

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?

@ghost
Copy link

ghost commented Apr 12, 2022

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
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?

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
@bsian03 bsian03 marked this pull request as draft January 9, 2024 21:57
@bsian03 bsian03 mentioned this pull request Jan 16, 2024
55 tasks
@coolcalcacol coolcalcacol marked this pull request as ready for review January 19, 2024 22:01
@bsian03
Copy link
Collaborator

bsian03 commented Oct 19, 2024

coolcalcacol and others added 2 commits October 20, 2024 01:14
Co-authored-by: bsian03 <chharry321@gmail.com>
Co-authored-by: TTtie <me@tttie.cz>
Co-authored-by: Donovan Daniels <hewwo@yiff.rocks>
@@ -166,6 +169,12 @@ class Message extends Base {
this.member = null;
}

if (data.attachments) {
for (const attachment of data.attachments) {
this.attachments.add(attachment, this);
Copy link
Collaborator

@bsian03 bsian03 Oct 21, 2024

Choose a reason for hiding this comment

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

This needs to be able to update/delete, or overwrite completely

Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like some of these properties can be modified

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.

6 participants