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.

Sign up for GitHub

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

Jump to bottom

fix s3 delete_many #38

Merged
merged 5 commits into from
Jul 14, 2017
Merged

fix s3 delete_many #38

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e308db0
fix s3 delete_many
itajaja Jul 14, 2017
6489b38
add tests and comments
itajaja Jul 14, 2017
79e4e0d
add comment
itajaja Jul 14, 2017
4cdc88f
use mock
itajaja Jul 14, 2017
5a0623d
install mock unconditionally
itajaja Jul 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions flask_annex/s3.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,15 @@ def delete(self, key):
self._client.delete_object(Bucket=self._bucket_name, Key=key)

def delete_many(self, keys):
objects = tuple({'Key': key} for key in keys)
Copy link
Contributor

Choose a reason for hiding this comment

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

probably worth noting that you're doing this first because keys may be an iterable

if not objects:
# this is not just an optimization, boto fails if the array
# is empty
return

self._client.delete_objects(
Bucket=self._bucket_name,
Delete={
'Objects': tuple({'Key': key} for key in keys),
},
Delete={'Objects': objects},
)

def get_file(self, key, out_file):
Expand Down
8 changes: 8 additions & 0 deletions tests/test_s3.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,14 @@ def test_get_upload_info_max_content_length(self, app, client):
def assert_app_config_content_length_range(self, conditions):
assert get_condition(conditions, 'content-length-range') == [0, 100]

def test_delete_many_empty_list(self, annex, monkeypatch):
def assert_not_called(*args, **kwargs):
assert(False)

monkeypatch.setattr(annex._client, 'delete_objects', assert_not_called)
Copy link
Contributor

Choose a reason for hiding this comment

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

why not just use a mock and actually assert it's not called, instead of this thing?

Copy link
Member Author

Choose a reason for hiding this comment

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

because I didn't think it was worth adding a dependency

Copy link

Choose a reason for hiding this comment

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

what dependency? isn't mock in python3?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes but tests run also on py2

Copy link
Contributor

Choose a reason for hiding this comment

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

I think adding that dev dep is fine/better

Copy link
Member Author

Choose a reason for hiding this comment

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

mkay


annex.delete_many(tuple())


class TestS3AnnexFromEnv(TestS3Annex):
@pytest.fixture
Expand Down