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 method to send post with multiple attached photos #313

Merged
merged 12 commits into from
Mar 30, 2024

Conversation

ryoryo25
Copy link
Contributor

Description

I've added a function send_images to send a post with multiple attached images.

Changes

  • Add function send_images to class Client & AsyncClient
  • Rewrite function send_image using send_images (to keep backward compatibility)
  • Add an example of usage of send_images

@MarshalX
Copy link
Owner

Hi! Thank you for contributing! I saw that many people looking for this method. Backward compatibility is awesome. A few notes before deep diving into code:

  • we write sync Client and then auto-generate async version. Looks like the generator failed. Probably you wrote the async version by hand. Could you pls fix it?
  • speaking of blobls upload in the async version could we try to upload blobs simultaneously? For example, using asyncio.gather. I mean and don't see the reason to wait to upload the previous image to upload a new one
  • check on 4 images and no more should be covered already by the model. I guess we could delete this if from here

@ryoryo25
Copy link
Contributor Author

ryoryo25 commented Mar 28, 2024

Thank you for reviewing my code!
I revised the following parts you mentioned.

  • Revert the async version of Client
  • Delete validation for the number of images
  • Modify the code generator of AsyncClient to generate code for uploading blobs simultaneously by using asyncio.gather
    uploads = [self.upload_blob(i) for i in images]
    converts to
    uploads = asyncio.gather(*[self.upload_blob(i) for i in images])

Copy link
Owner

@MarshalX MarshalX left a comment

Choose a reason for hiding this comment

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

thank you! pls check why CI still fails

packages/atproto_client/client/client.py Outdated Show resolved Hide resolved
packages/atproto_client/client/client.py Outdated Show resolved Hide resolved
packages/atproto_client/client/client.py Outdated Show resolved Hide resolved
packages/atproto_codegen/clients/generate_async_client.py Outdated Show resolved Hide resolved
examples/send_images.py Outdated Show resolved Hide resolved
@ryoryo25
Copy link
Contributor Author

I've fixed the parts you mentioned. I think CI would be passed.

@MarshalX MarshalX changed the title Add function to send post with multiple attached photos Add method to send post with multiple attached photos Mar 30, 2024
Copy link
Owner

@MarshalX MarshalX left a comment

Choose a reason for hiding this comment

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

great job!

@MarshalX MarshalX merged commit e069c04 into MarshalX:main Mar 30, 2024
23 checks passed
@ryoryo25 ryoryo25 deleted the sending-multiple-photos branch March 30, 2024 13:56
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.

2 participants