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 CreateAttachmentStream type #2774

Closed

Conversation

mkrasnitski
Copy link
Collaborator

Addresses #2690 by adding a type which allows lazily creating attachments which only get resolved at request time. The Clone requirement means that we can't store the Reader types directly, which is unfortunate but not too bad.

Draft for now because I don't know how to concisely add support to the create_* builders. Adding support to the edit_* builders was easy by just adding an EditAttachments::add_stream method. However, the create_* builders have three separate methods for adding attachments: add_file, add_files, and files. Duplicating all three to accommodate the streaming variant seems quite noisy.

@github-actions github-actions bot added the builder Related to the `builder` module. label Feb 19, 2024
@GnomedDev
Copy link
Member

I don't see how this solves the linked issue. These types still require loading all the files into memory before sending them, instead of streaming them, it just defers the load to the builder execution instead of the builder creation. Implementing proper streaming would probably require dropping down to hyper, but I'm not sure.

@mkrasnitski
Copy link
Collaborator Author

I guess I was thinking about long-lived builders which keep their data around in memory. If we want to use readers directly, we'll have to drop the Clone requirement which would require separating out the attachment methods for builders so that the builders themselves can still remain Clone.

@mkrasnitski
Copy link
Collaborator Author

Closing this for now as it isn't the solution we really want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builder Related to the `builder` module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants