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

feat: add configurable crc32c checksumming for downloads #135

Merged
merged 15 commits into from Jul 7, 2020
Merged

Conversation

andrewsg
Copy link
Contributor

No description provided.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 19, 2020
@andrewsg
Copy link
Contributor Author

Received a design suggestion to move the argument to the class constructor for the download class; looking into refactoring that now.

Copy link
Member

@frankyn frankyn left a comment

Choose a reason for hiding this comment

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

Overall, I think it LGTM, added a few comments for changes.

google/resumable_media/requests/download.py Outdated Show resolved Hide resolved
google/resumable_media/requests/download.py Outdated Show resolved Hide resolved
google/resumable_media/requests/download.py Show resolved Hide resolved
@andrewsg
Copy link
Contributor Author

@frankyn PTAL

Copy link
Member

@frankyn frankyn left a comment

Choose a reason for hiding this comment

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

Overall LGTM, pending approval @crwilcox as well.

google/resumable_media/_download.py Show resolved Hide resolved
google/resumable_media/_download.py Show resolved Hide resolved
@andrewsg
Copy link
Contributor Author

Fixed merge conflict and addressed comments.

Copy link
Member

@frankyn frankyn left a comment

Choose a reason for hiding this comment

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

I missed a potential refactor, adding that in.

google/resumable_media/_download.py Outdated Show resolved Hide resolved
google/resumable_media/requests/download.py Outdated Show resolved Hide resolved
@andrewsg
Copy link
Contributor Author

Thanks for the suggestion, I was able to remove some duplication of code between Download and RawDownload and tighten it up a bit in general. PTAL

Copy link
Member

@frankyn frankyn left a comment

Choose a reason for hiding this comment

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

I have one more question on crc32c support for google-crc32c and how users are being warned.

Thanks for the refactor, it looks much cleaner!

google/resumable_media/_download.py Outdated Show resolved Hide resolved
google/resumable_media/requests/_helpers.py Show resolved Hide resolved
@andrewsg
Copy link
Contributor Author

andrewsg commented Jul 6, 2020

@frankyn fixed coverage issue; PTAL

Copy link
Member

@frankyn frankyn left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @andrewsg,

pending kokoro failure:

Formatting issue:

flake8 google/resumable_media tests
black --check google/resumable_media tests
would reformat /tmpfs/src/github/google-resumable-media-python/tests/unit/requests/test_download.py
Oh no! 💥 💔 💥
1 file would be reformatted, 24 files would be left unchanged.

@tseaver tseaver deleted the crc32c branch July 29, 2020 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants