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

Document multi file upload #3860

Merged
merged 16 commits into from
Oct 21, 2021
Merged

Document multi file upload #3860

merged 16 commits into from
Oct 21, 2021

Conversation

MinnDevelopment
Copy link
Contributor

Since @IanMitchell showed us multi attachment support will be added to the client, I see no reason to keep this undocumented any longer.

I'm not sure if documenting the uploading mechanism only under Create Message makes sense, maybe a dedicated section outside of endpoint docs would be better. I do think it makes sense to just link to the specific section rather than duplicating it 3 times.

docs/resources/Webhook.md Outdated Show resolved Hide resolved
docs/resources/Channel.md Outdated Show resolved Hide resolved
@typpo
Copy link
Contributor

typpo commented Sep 29, 2021

This behavior may be subject to change, so going to hold off merging for now

@typpo typpo added the not released This issue or PR is referencing a change that is not yet widely released and/or subject to change. label Sep 29, 2021
@@ -311,6 +311,7 @@ We highly recommend checking out our [Community Resources](#DOCS_TOPICS_COMMUNIT
## Create Interaction Response % POST /interactions/{interaction.id#DOCS_INTERACTION_RECEIVING_AND_RESPONDING/interaction}/{interaction.token#DOCS_INTERACTION_RECEIVING_AND_RESPONDING/interaction-object}/callback

Create a response to an Interaction from the gateway. Takes an [interaction response](#DOCS_INTERACTION_RECEIVING_AND_RESPONDING/interaction-response-object).
This endpoint also supports file attachments similar to the webhook endpoints. Refer to [Uploading Files](#DOCS_RESOURCES_CHANNEL/create-message-uploading-files) for details on uploading files and `multipart/form-data` requests.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a case of you can include file attachments with the initial response for interactions received over the gateway but not for interactions received over REST or does this pr just not document it for rest responses?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the file uploads haven't been documented for that response at all, I excluded it intentionally. Theoretically, it would work identically but with a multipart body response.

@IanMitchell IanMitchell self-requested a review October 14, 2021 16:40
docs/Reference.md Outdated Show resolved Hide resolved
docs/Reference.md Outdated Show resolved Hide resolved
Copy link
Contributor

@IanMitchell IanMitchell left a comment

Choose a reason for hiding this comment

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

Looks fantastic Minn, thank you for this!

docs/Reference.md Outdated Show resolved Hide resolved
> This endpoint supports both `application/json` and `multipart/form-data` bodies. When uploading files the `multipart/form-data` content type must be used.
> Note that in multipart form data, the `embeds` and `allowed_mentions` fields cannot be used. You can pass a stringified JSON body as a form value as `payload_json` instead.
> **If you supply a `payload_json` form value, all fields except for `file` fields will be ignored in the form data**.
> Note that when sending a message, you must provide a value for at **least one of** `content`, `embeds`, or `file`.
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think of saying "attachments" (no code backticks) instead of file? cc @msciotti

Feels like since it isn't the parameter name, we should style it differently

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it? Looking at the param table below, there is an attachments parameter

> Note that in multipart form data, the `embeds`, `allowed_mentions`, and `attachments` fields cannot be used. You can pass a stringified JSON body as a form value as `payload_json` instead.
> **If you supply a `payload_json` form value, all fields except for `file` fields will be ignored in the form data**.
Refer to [Uploading Files](#DOCS_REFERENCE/uploading-files) for details on attachments and `multipart/form-data` requests.
Any provided files will be **appended** to the message. To remove or replace files you will have to supply the `attachments` field which specifies the files to retain on the message after edit.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what the API version callout style guide is, but do we want to include a note that in v10 users will need to patch with the complete attachment array?

Copy link
Contributor

Choose a reason for hiding this comment

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

implying that there is a style guide :^)

I think get active channel threads has a deprecation notice tho

Copy link
Contributor

Choose a reason for hiding this comment

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

I think a warn block would do when functionality is expected to change in the next version

docs/resources/Channel.md Show resolved Hide resolved
docs/resources/Channel.md Outdated Show resolved Hide resolved
docs/Reference.md Outdated Show resolved Hide resolved
docs/Reference.md Outdated Show resolved Hide resolved
@IanMitchell
Copy link
Contributor

Looks good to me, pinged Mason about possibly taking a pass

@IanMitchell IanMitchell removed the not released This issue or PR is referencing a change that is not yet widely released and/or subject to change. label Oct 21, 2021
Copy link
Contributor

@msciotti msciotti left a comment

Choose a reason for hiding this comment

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

Looks great to me, thank you Minn. Helpful to split it out into its own section in the reference as well.

@msciotti msciotti merged commit 4967eca into discord:master Oct 21, 2021
@MinnDevelopment MinnDevelopment deleted the feature/multi-upload branch October 21, 2021 23:00
Erk- added a commit to Erk-/twilight that referenced this pull request Oct 22, 2021
This brings support for multiple attachments up to date with
discord/discord-api-docs#3860 with the
exception of support for setting description initially as there is not
support for it yet.
Nihlus added a commit to Remora/Remora.Discord that referenced this pull request Oct 28, 2021
7596ff pushed a commit to twilight-rs/twilight that referenced this pull request Nov 20, 2021
This brings support for multiple attachments up to date with
discord/discord-api-docs#3860 with the
exception of support for setting description initially as there is not
support for it yet.
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.

None yet

9 participants