-
-
Notifications
You must be signed in to change notification settings - Fork 382
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
accept bytearray and memoryview as input to write in s3 submodule #293
Conversation
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.
Thank you for your contribution. Left you a minor comment.
smart_open/s3.py
Outdated
@@ -477,8 +477,14 @@ def write(self, b): | |||
|
|||
There's buffering happening under the covers, so this may not actually | |||
do any HTTP transfer right away.""" | |||
if not isinstance(b, six.binary_type): | |||
raise TypeError("input must be a binary string, got: %r", b) | |||
import sys |
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.
Please move this stuff to module scope, and make it an internal constant, e.g. _BINARY_TYPES.
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.
Done
Looking good. Could you please add a unit test that exercises your new feature? |
Added a test. Tested to make sure it fails without the fix. |
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.
Looks good. Left you some minor nitpicks.
Hi Michael, can you give me some idea of when this might land in pypi? |
We just did a bugfix release, I suppose we could do another one in a week or so. Are you in a rush? |
It would be helpful, otherwise I'll have to live off a branch for a bit. thanks! |
Co-Authored-By: bmizhen-exos <43822460+bmizhen-exos@users.noreply.github.com>
No, that's pretty much it, congrats on your first contribution to smart_open! 🥇 |
Thank you!
…On Thu, Apr 18, 2019, 11:35 PM Michael Penkov ***@***.***> wrote:
Merged #293 <#293>
into master.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#293 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AKOK27AKR2BP5JSGXRKS4V3PRE4YDANCNFSM4HGDAC6Q>
.
--
Brokerage services offered through Exos Securities LLC, member of
SIPC <http://www.sipc.org/> / FINRA <http://www.finra.org/>. For important
disclosures, click here <https://www.exosfinancial.com/disclosures>.
|
Released 1.8.3 |
Thank you!
…On Fri, Apr 26, 2019, 3:26 AM Michael Penkov ***@***.***> wrote:
Released 1.8.3
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#293 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AKOK27EDNNJM4QQKFBFZA6TPSKVDRANCNFSM4HGDAC6Q>
.
--
Brokerage services offered through Exos Securities LLC, member of
SIPC <http://www.sipc.org/> / FINRA <http://www.finra.org/>. For important
disclosures, click here <https://www.exosfinancial.com/disclosures>.
|
Hello, I'd like to add ability to handle memoryview objects.
https://docs.python.org/2/c-api/buffer.html
I encountered this when attempting to wrap a s3 stream in a io.BufferedWriter like this