-
Notifications
You must be signed in to change notification settings - Fork 27
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
Support for ACL, File Content Type, Cache Max Age, and user Metadata added #171
Support for ACL, File Content Type, Cache Max Age, and user Metadata added #171
Conversation
Codecov Report
@@ Coverage Diff @@
## master #171 +/- ##
==========================================
+ Coverage 92.10% 92.16% +0.05%
==========================================
Files 32 33 +1
Lines 1774 1787 +13
==========================================
+ Hits 1634 1647 +13
Misses 140 140
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
This pull request introduces 1 alert when merging 98ffdf5 into ef105f7 - view on LGTM.com new alerts:
|
@@ -15,12 +17,14 @@ def __init__(self, *routers: Router) -> None: | |||
async def __call__(self, scope: Scope, receive: Receive, send: Send): | |||
for router in self.routers: | |||
try: | |||
asgi = await router(scope, receive=receive, send=send) | |||
response: t.Any | None = await router( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a tip. In future, you must be careful to use syntax that will satisfy the tests in Py3.7
, Py3.8
, P3.9
, and Py3.10
. I run your branch and everything should be fine, just remove the import typing as t
from file because is unused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'll be careful. Done the changes. Hope everything will be good now. Please have a look and merge it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. You have to wait @dantownsend for merging.
@dantownsend , Please merge this. |
I think it's almost there. I left a couple of comments. My only other concern is before we were assuming the bucket would be private, so all of the URLs are signed. But now it could be a public bucket, so there's no need to sign the URLs. In the future we could add something like a |
If you like I'll make the last couple of changes, and will release it. |
Please do the change what you feel necessary and recommends and then do release. |
Merged in #177 |
Currently, The visibility and accessibility of the file was private. But now user can define the visibility and accessibility (default is private).
Currently, No any file content-type was assigned by the piccolo-api, but now it is automatically assigned by the piccolo-api.
Currently, There is no option for defining the file cache age, but now it can be defined.
Currently, there was no support for the user defined metadata for the file, but now it can be defined.