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

Add ability to send attachments in an interaction initial response #971

Merged

Conversation

tandemdude
Copy link
Contributor

Summary

Adds the ability to pass attachments with create_initial_response

Checklist

  • I have run nox and all the pipelines have passed.
  • I have made unittests according to the code I have added/modified/deleted.

Related issues

@davfsa davfsa added the enhancement New feature or request label Jan 22, 2022
@FasterSpeeding
Copy link
Collaborator

I'ma see if i can work on the rest server equivalent logic for this today

@FasterSpeeding
Copy link
Collaborator

Btw I do have a prototype impl of sending attachments on rest but i'm going to be waiting for discord to push a change they said they'd be pushing soon:tm: so if you remove the interaction server from this pr and switch out to handling de-duplicating the logic by moving out the payload building logic to a separate function instead of making the create_message logic work for create initial response this can be merged without the server logic

(and i'll do the server logic in another pr)

@FasterSpeeding
Copy link
Collaborator

Sry for the delay

@FasterSpeeding
Copy link
Collaborator

FasterSpeeding commented Jan 27, 2022

For a bit more specific detail on how I think this should be handled: instead of modifying _create_message to also handle initial response specific logic I think we would be better off taking the logic for building the message response payload and doing something like

def _build_message(self, message_body: JSONObjectBuilder, ...) -> Optional[URLEncodedFormBuilder]:

where _build_message should cover both edit and create semantics, and if it returns a URLEncodedFormBuilder then the calling logic should handle inserting the "payload_json" field before making the request

@tandemdude tandemdude force-pushed the feature/initial-response-attachments branch from f0c8167 to aa9bcdd Compare January 27, 2022 20:40
@tandemdude
Copy link
Contributor Author

Tests are still a TODO but a pre-emptive review would be welcomed @FasterSpeeding 😉

hikari/impl/rest.py Outdated Show resolved Hide resolved
hikari/impl/rest.py Outdated Show resolved Hide resolved
hikari/impl/rest.py Show resolved Hide resolved
hikari/impl/rest.py Outdated Show resolved Hide resolved
@FasterSpeeding
Copy link
Collaborator

Meow

@hypergonial
Copy link
Contributor

Woof

@tandemdude tandemdude force-pushed the feature/initial-response-attachments branch from b505045 to 5eec844 Compare January 28, 2022 15:10
hikari/impl/rest.py Outdated Show resolved Hide resolved
hikari/impl/rest.py Outdated Show resolved Hide resolved
hikari/impl/rest.py Outdated Show resolved Hide resolved
hikari/impl/rest.py Show resolved Hide resolved
hikari/impl/rest.py Show resolved Hide resolved
@davfsa davfsa merged commit b754ee2 into hikari-py:master Jan 29, 2022
@tandemdude tandemdude deleted the feature/initial-response-attachments branch January 29, 2022 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants