Skip to content
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

Fix off by one, raise ValueError on closed file and set readable = True #69

Merged
merged 3 commits into from
Nov 28, 2017

Conversation

hoffmann
Copy link
Collaborator

No description provided.

@coveralls
Copy link

coveralls commented Nov 23, 2017

Coverage Status

Coverage decreased (-6.4%) to 82.985% when pulling 69d320c on hoffmann:master into 73d7b42 on mbr:master.

Copy link
Collaborator

@criemen criemen left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!
Don't care about coveralls, as for branches not in the mbr repo no azure tests are run, thus decreasing the coverage.

with map_azure_exceptions(key=self.key):
if size < 0:
size = self.size - self.pos

end = min(self.pos + size - 1, self.size)
end = min(self.pos + size - 1, self.size - 1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we have a test which fails on current master and succeeds with the change?

Copy link
Collaborator Author

@hoffmann hoffmann Nov 23, 2017

Choose a reason for hiding this comment

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

I'm not sure. The azure API does not care if you request more than the actual size. It just gives you the all the bytes it can. So in my opinion the current implementation should have also worked fine. So the new implementation is only more correct, but behaves same as the previous one.

block_blob_service = BlockBlobService(account_name=ACCOUNT_NAME, account_key=ACCOUNT_KEY)
b = block_blob_service.get_blob_properties("parquet", "test")
size = b.properties.content_length
b = block_blob_service.get_blob_to_bytes("parquet", "test", start_range=size-1, end_range=size)
b.content

Copy link
Collaborator

@criemen criemen left a comment

Choose a reason for hiding this comment

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

Yeah, that sounds sensible.
Can you add a Changelog entry? Then I will merge the PR.

@coveralls
Copy link

coveralls commented Nov 27, 2017

Coverage Status

Coverage decreased (-6.4%) to 82.985% when pulling c31aa84 on hoffmann:master into 73d7b42 on mbr:master.

@criemen criemen merged commit 8a95539 into mbr:master Nov 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants