Skip to content

Async storage buckets #61

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

Merged
merged 8 commits into from
Oct 23, 2021
Merged

Async storage buckets #61

merged 8 commits into from
Oct 23, 2021

Conversation

anand2312
Copy link
Contributor

This PR adds async support to the storage buckets API by switching to httpx. All sync methods can now also be called with async/await syntax, by passing is_async=True to the StorageBucketAPI constructor (this value defaults to False).

Other smaller changes:

  • Removed version pin from pre-commit-config (would error out if you did not have a local install of Python 3.7)
  • Added name, public arguments to StorageBucketAPI.create_bucket method
  • Better error propagation by raising a StorageException, instead of excepting all errors and printing them out.
  • Type-hinted the storage_bucket_api.py file (I might've gone a little overboard with that, sorry)

@J0 J0 added the hacktoberfest Hacktoberfest 2022 label Oct 13, 2021
@leynier
Copy link
Contributor

leynier commented Oct 13, 2021

Personally, I don't think sync/async support should be done on the same class depending on a parameter in initialization.

The reason is that a method is made Awaitable or not, through a Union, and that causes that to apply the await or not to the method, it is necessary to use the cast by the type annotation.

I think that the path that the HTTPX itself follows is a good one, that of using two different classes.

@anand2312
Copy link
Contributor Author

anand2312 commented Oct 13, 2021

I agree, it's not the neatest solution out there, I'm open to suggestions. I was mainly trying to reduce the amount of code duplication that would occur with two separate classes.
I can split it into two classes if more people are leaning towards that direction

@leynier
Copy link
Contributor

leynier commented Oct 17, 2021

I have been investigating and I think this way is very good. I will test it with gotrue-py to see the result.

The idea is to use unasync and httpx, unasync generates the synchronous client, so implementing the asynchronous client with some special features unasync when building the package that is uploaded to pypi will generate the synchronous client.

https://sethmlarson.dev/blog/2020-06-27/designing-libraries-for-async-and-sync-io
https://github.com/sethmlarson/pycon-async-sync-poster
https://github.com/python-trio/unasync
https://github.com/encode/httpx

Copy link
Contributor

@J0 J0 left a comment

Choose a reason for hiding this comment

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

A few nits but otherwise looks good to me. I think it's quite well put together so kudos on that. Thanks much!

Ping me(J0) on discord if you haven't received any swag yet.

@J0
Copy link
Contributor

J0 commented Oct 22, 2021

@leynier I think you raise some great points about splitting the code into separate async/sync clients. The links provided are really helpful so thank you for pointing this out!

We can poll the community and see what they'd prefer and switch if necessary but for now I hope to accept this PR. What do both of you think?

@leynier
Copy link
Contributor

leynier commented Oct 22, 2021

@leynier I think you raise some great points about splitting the code into separate async/sync clients. The links provided are really helpful so thank you for pointing this out!

We can poll the community and see what they'd prefer and switch if necessary but for now I hope to accept this PR. What do both of you think?

All good for me 👌🏼

@dreinon
Copy link
Contributor

dreinon commented Oct 22, 2021

Hey @J0 @leynier @anand2312!
I also think @leynier made a good point on using unasync, and since gotrue will use it too, it would add consistency to the libraries.

Nevertheless, this PR is such a good job, so I'm thinking in 2 options:

  1. We could go forth accepting this PR and then maybe @leynier can open another one.
  2. Maybe @leynier could open another PR from his own branch to @anand2312's branch, and if they accept it, then the changes would appear in this PR and we could accept the "final" version.

What do you all think?

@leynier
Copy link
Contributor

leynier commented Oct 22, 2021

I'm willing to help in whatever is needed, so, I adjust to what @J0 and @anand2312 decide.

@anand2312
Copy link
Contributor Author

@dreinon @J0 I think I'd rather merge this and move it to unasync when @leynier is able to, as this PR also (accidentally) includes the fix for #65 ; I wanna move onto adding async support to more of the library, so I'll make sure to implement unasync next time

@dreinon
Copy link
Contributor

dreinon commented Oct 23, 2021

Lgtm

@J0
Copy link
Contributor

J0 commented Oct 23, 2021

Sounds good to me! Going to merge this in.

@J0
Copy link
Contributor

J0 commented Oct 23, 2021

Thanks everyone for your inputs 😄

@J0 J0 merged commit 6469ad5 into supabase:develop Oct 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest Hacktoberfest 2022
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants