Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Extend read() method to support no-args and negative sizes (meaning readall) #2081

Open
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants

rivimey commented Feb 6, 2014

This patch updates the read() method in the s3 transfer code to support read() as well as read(n). I wanted to be able to use the python zipfile code, which calls both forms, and the existing code fails with a syntax error when used this way.

Owner

danielgtaylor commented Feb 19, 2014

@rivimey can you please add a unit test to cover the new cases (no args and negative size). After that I don't mind merging this in so that it behaves more like a file object.

rivimey commented May 2, 2014

Sorry for the delay in responding. I have pushed a change which seems reasonable but I don't know how to run the tests without messing up my own environment. The test case I added seems logical enought though.

rivimey commented May 10, 2015

@danielgtaylor would you be able to have a look at merging this?

@signalpillar signalpillar commented on the diff May 10, 2015

boto/s3/keyfile.py
@@ -84,9 +84,17 @@ def seek(self, pos, whence=os.SEEK_SET):
self.location = pos
- def read(self, size):
- self.location += size
- return self.key.read(size)
+ def read(self,*args):
@signalpillar

signalpillar May 10, 2015

as I understand it would be better to use named parameter here instead of vararg.

def read(self, size=-1):
  1. Could you please document touched method
Read at most size bytes from the file (less if the read hits EOF before obtaining size bytes).

:param size: Size of the content to read in bytes. If the size argument is negative or omitted, read all data until EOF is reached. 
:type size: int
:return: .... `None` is returned when EOF is encountered.
:rtype: ?

@signalpillar signalpillar commented on the diff May 10, 2015

boto/s3/keyfile.py
@@ -84,9 +84,17 @@ def seek(self, pos, whence=os.SEEK_SET):
self.location = pos
- def read(self, size):
- self.location += size
- return self.key.read(size)
+ def read(self,*args):
+ if len(args) == 0 or (len(args) == 1 and args[0] < 0):
+ rv = self.key.read()
+ if rv != None:
+ self.location += len(rv)
+ else:
@signalpillar

signalpillar May 10, 2015

Not sure this branching is required. If key follows file protocol we can pass negative values too.
So the resulted should be like

result = self.key.read(size)
if result is not None:
    self.location  += len(result)
return result

@signalpillar signalpillar commented on the diff May 10, 2015

boto/s3/keyfile.py
@@ -84,9 +84,17 @@ def seek(self, pos, whence=os.SEEK_SET):
self.location = pos
- def read(self, size):
- self.location += size
- return self.key.read(size)
+ def read(self,*args):
+ if len(args) == 0 or (len(args) == 1 and args[0] < 0):
+ rv = self.key.read()
+ if rv != None:
@signalpillar

signalpillar May 10, 2015

from PEP8 E711 (^) comparison to None should be ‘if cond is None:’

rivimey commented May 10, 2015

Thank you for the review. However, I think I will bow out... not pique, but simply that I'm no python expert and there are issues here that go beyond me.

My sole intention in making this change was to enable the code to be used by the python zlib wrapper, which in turn assumes a standard python lib compatible 'read' method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment