upload_file() and download_file() for Bucket and Object #243
Conversation
rayluo
added
enhancement
| def bucket_download_file(self, Key, Filename, | ||
| ExtraArgs=None, Callback=None, Config=None): | ||
| """Download an S3 object to a file.""" | ||
| transfer = S3Transfer(self.meta.client, Config) |
There was a problem hiding this comment.
Is there a reason we can't just use the .meta.client methods for upload_file/download_file? Also seems like it would cut down on the duplicate boiler plate of creating an S3Transfer object for each of the four functions you've added.
There was a problem hiding this comment.
It seems the S3Transfer provides more advanced features. Besides, the pre-existing upload_file() and download_file() also use S3Transfer so I follow the pattern.
Understood. You are talking about the already-injected self.meta.client, not the raw botocore client. Testing. Will send out new PR soon.
rayluo
force-pushed
the
feature-s3-transfer-methods-for-bucket-and-object
branch
from
a87a6e9
to
e83f72f
Compare
|
|
Looks good!
|
|
||
| def bucket_upload_file(self, Filename, Key, | ||
| ExtraArgs=None, Callback=None, Config=None): | ||
| """Upload a file to an S3 object.""" |
There was a problem hiding this comment.
It would be cool if we can add a little more to all of these docstrings. These docstrings will get publicly exposed in our documentation and it would be nice to have parameters documented and maybe even an example of how to use it.
|
It looks good to me. Had a little nit picky comment about the docstrings. I felt that it would be good to build up to help the exposure of these methods as they will show up in the html along with its docstrings in the list of available actions for the Bucket and Key. Otherwise, 🚢 |
rayluo
force-pushed
the
feature-s3-transfer-methods-for-bucket-and-object
branch
from
e83f72f
to
d648c9d
Compare
| Detailed explanations and examples can be found at S3Transfer's Usage page | ||
| http://boto3.readthedocs.org/en/latest/reference/customizations/s3.html#usage | ||
| """ | ||
| for family, funcions in functions_to_be_patched.items(): |
There was a problem hiding this comment.
We should generally avoid having code like this run in the global scope.
There's already very similar text in each of the docstrings. I don't think it's that big of a downside if we have to repeat some documentation.
If we really want to save some copy/paste of docs, we should be doing it in a way that avoids code at the module level.
However, I'd vote to just keep it simple and add the docstrings directly.
rayluo
force-pushed
the
feature-s3-transfer-methods-for-bucket-and-object
branch
from
d648c9d
to
e0117f6
Compare
|
The latest feedback has been incorporated. That CI error is caused by another issue not relevant to this PR. |
|
What is the issue related to the CI error? |
|
It is relevant to this PR in botocore. boto/botocore#645 |
|
We generally don't merge things unless the travis builds pass. Wouldn't rebasing against develop get the builds passing? Once you're able to get travis passing feel free to merge. |
|
@jamesls |
|
OK we |
Rely on the already-injected meta.client rather then S3Transfer. Unit test, function test, integration test and CHANGELOG are also updated.
rayluo
force-pushed
the
feature-s3-transfer-methods-for-bucket-and-object
branch
from
e0117f6
to
a59b6a0
Compare
|
Looks good to merge. I would not worry about the coveralls check not running. |
|
|
…ucket-and-object upload_file() and download_file() for Bucket and Object
rayluo
deleted the
feature-s3-transfer-methods-for-bucket-and-object
branch
Unit test, function test, integration test and CHANGELOG are also updated.