Download Review File Attachments as tarball
Review Request #6402 — Created Oct. 5, 2014 and submitted
Download File Attachments as tarball:
- only working with files stored on host filesystem and Amazon S3.
- now compresses into .tar.gz file instead.
- properly streams compressed file back to client.
- Download Attachments and Download Diff now under Downloads
menu dropdown in review request editor.- URL changed from: r/<requestID>/file/zip/ -> r/<requestID>/file/tar/
Tested on:
- Linux Mint 17: Chromium 37.0.2062.120
- Linux Mint 17: Firefox 32.0.3
- Windows 8.1: Chrome 38.0.2125.101
- Windows 8.1: Firefox 32.0.3
- Windows 8.1: Internet Explorer 11
Tested also with curl to see if file is streaming as expected.Review requests with small files, large files (2mb+), and mix of both, tested and working with host file system and Amazon S3 storage option.
Description | From | Last Updated |
---|---|---|
Col: 80 E501 line too long (100 > 79 characters) |
reviewbot | |
Col: 1 E302 expected 2 blank lines, found 1 |
reviewbot | |
local variable 'zip_path' is assigned to but never used |
reviewbot | |
Col: 9 E265 block comment should start with '# ' |
reviewbot | |
Col: 19 E702 multiple statements on one line (semicolon) |
reviewbot | |
Col: 36 E703 statement ends with a semicolon |
reviewbot | |
Col: 9 E265 block comment should start with '# ' |
reviewbot | |
local variable 'zip_filename' is assigned to but never used |
reviewbot | |
Col: 80 E501 line too long (82 > 79 characters) |
reviewbot | |
list comprehension redefines 'file_attachment' from line 621 |
reviewbot | |
Col: 49 W291 trailing whitespace |
reviewbot | |
Col: 27 W291 trailing whitespace |
reviewbot | |
Col: 38 W291 trailing whitespace |
reviewbot | |
Col: 9 E113 unexpected indentation |
reviewbot | |
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot | |
list comprehension redefines 'file_attachment' from line 618 |
reviewbot | |
There should be a single blank line before a block |
brennie | |
list comprehension redefines 'file_attachment' from line 619 |
reviewbot | |
This is in the diff in https://reviews.reviewboard.org/r/6432/ You should mark this RR as depends on 6432 and base your RR … |
brennie | |
Col: 5 E303 too many blank lines (2) |
reviewbot | |
Col: 6 E111 indentation is not a multiple of four |
reviewbot | |
Col: 6 E113 unexpected indentation |
reviewbot | |
Col: 6 E265 block comment should start with '# ' |
reviewbot | |
Col: 6 E303 too many blank lines (2) |
reviewbot | |
Col: 9 E265 block comment should start with '# ' |
reviewbot | |
list comprehension redefines 'file_attachment' from line 629 |
reviewbot | |
Col: 80 E501 line too long (82 > 79 characters) |
reviewbot | |
Col: 21 E128 continuation line under-indented for visual indent |
reviewbot | |
Col: 21 E128 continuation line under-indented for visual indent |
reviewbot | |
Col: 5 E303 too many blank lines (2) |
reviewbot | |
Col: 80 E501 line too long (87 > 79 characters) |
reviewbot | |
list comprehension redefines 'file_attachment' from line 634 |
reviewbot | |
Col: 40 W291 trailing whitespace |
reviewbot | |
Col: 9 E128 continuation line under-indented for visual indent |
reviewbot | |
Col: 56 E502 the backslash is redundant between brackets |
reviewbot | |
Col: 72 E502 the backslash is redundant between brackets |
reviewbot | |
Col: 21 E128 continuation line under-indented for visual indent |
reviewbot | |
Col: 21 E128 continuation line under-indented for visual indent |
reviewbot | |
Col: 13 E128 continuation line under-indented for visual indent |
reviewbot | |
list comprehension redefines 'file_attachment' from line 635 |
reviewbot | |
Col: 41 E502 the backslash is redundant between brackets |
reviewbot | |
Col: 9 E128 continuation line under-indented for visual indent |
reviewbot | |
Col: 13 E128 continuation line under-indented for visual indent |
reviewbot | |
list comprehension redefines 'file_attachment' from line 636 |
reviewbot | |
undefined name 'eview_request_id' |
reviewbot | |
list comprehension redefines 'file_attachment' from line 636 |
reviewbot | |
As mentioned earlier, can you pull the latest master and rebase? |
david | |
One too many blank lines here. |
david | |
You should use the 'six' version of StringIO for python 3.x compat: from django.utils.six.moves import cStringIO as StringIO ... s … |
david | |
Revert this change. |
david | |
Since you only iterate over file_paths once, how about combining these? Then your loop belowe would look something like: for … |
david | |
Instead of hard-coding './reviewboard/htdocs' (which won't work in a deployed server), it should use siteconfig.get('site_media_root') |
david | |
You should use os.path.join to create file paths. |
brennie | |
You're leaking the file here. This should be: with open(file_path, 'r') as f: file_contents = f.read() |
david | |
It's very weird to mix formatting styles here, plus you're doing two format operations where you only need one. How … |
david | |
Suggestion: why not bool(file_attachments)? Empty lists will evaluate to false and so will None. |
brennie | |
list comprehension redefines 'file_attachment' from line 636 |
reviewbot | |
Can we make the URL /file/zip/ ? That way it fits in with the existing /file/N/ URLs. |
david | |
Is this necessary? Anything on PyPI should automatically be included... |
david | |
Why not wrap it in () so that you don't need the \\, i.e. response['Content-Disposition'] = ( 'attachment; filename=review-attachments-%s' % … |
brennie | |
Shouldn't this have a .zip extension? |
brennie | |
Docstrings should always be in this format: """Single-line summary.""" or: """Single-line summary. Multi-line description. """ with no blank line after. … |
chipx86 | |
I'd just put .all() at the end of this one. No point doing it below. |
chipx86 | |
Blank line between these. |
chipx86 | |
Blank line between these. |
chipx86 | |
The point of the storage backends is that you should never have to do logic like this. Plus, there is … |
chipx86 | |
As above, we're not actually taking advantage of the streaming here. What you're doing is reading everything into memory and … |
chipx86 | |
"review-attachments" is a misleading prefix. This is for attachments on the review request, not reviews. However, we don't want the … |
chipx86 | |
We should probably have both of these within a "Download" menu, like we have with "Update." |
chipx86 | |
I know the other links are hard-coded, but we should be using {% url download-zip-attachments review_request.display_id %} here. |
chipx86 | |
Should have a comma at end of line. That way future dependancies will only be a 1-line change. |
brennie | |
local variable 'total_size' is assigned to but never used |
reviewbot | |
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot | |
If you're only compressing as tar.gz you won't need zipstream. |
brennie | |
'os' imported but unused |
reviewbot | |
local variable 'siteconfig' is assigned to but never used |
reviewbot | |
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot | |
This should go under review_request_urls. |
chipx86 | |
No spaces before/after the """. This should read more like a sentence. In fact, it should really be fleshed out, … |
chipx86 | |
I know we have some old docstrings in here, but this should be of the following format: """One-line summary (< … |
chipx86 | |
This and all the other functions should go in a utility object that wraps tarfile.TarFile. This view should be able … |
chipx86 | |
Docstrings shouldn't have a space after the first triple quote, e.g. """Build TarInfo...""" Same below |
brennie | |
We should just let a link be a link. I think, given other issues reported during today's meeting, that links … |
chipx86 | |
These don't look like they were meant to be in this change? |
chipx86 | |
undefined name 'time' |
reviewbot | |
This must be added alphabetically to the list of imports (after reviewboard.attachments.models.) |
chipx86 | |
How about "Downloads all file attachments on a review request as a tarball." ? |
chipx86 | |
"... are downloaded as a tarball." Not sure we need to talk about what isn't supported. |
chipx86 | |
Can you put the arguments on the first line? If they need to wrap, do: response[...] = ('...' % out_filename) |
chipx86 | |
Not sure we need this. The template can just check the file attachments list. I think we just want to … |
chipx86 | |
I wasn't clear before, but what I meant in the previous review is that the templates can just check file_attachments, … |
chipx86 | |
No space after """. |
chipx86 | |
Too many backticks. Generally, though, we don't add this sort of doc to the docstrings. This should be more higher-level, … |
chipx86 | |
No need to say all the things being outputted. Just say what the function does at a high level, and … |
chipx86 | |
These should be class-level constants. |
chipx86 | |
Will these ever be changed between instances? I can understand that it might be beneficial to keep the BLOCK_SIZE as … |
ML mloyzer | |
This should be written more like how you'd say it to someone. "Build a TarInfo object representing one file in … |
chipx86 | |
This should have a try/except for each thing, since a storage backend may implement one but not the other. It … |
chipx86 | |
We should yield before we close. |
chipx86 | |
Same comment as above. This shouldn't know how it's going to be used (the StreamingHttpResponse part). Just go into the … |
chipx86 | |
Blank line between these. |
chipx86 | |
yield isn't a function, so you shouldn't use parens. |
chipx86 | |
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot |
-
Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py setup.py reviewboard/reviews/urls.py Ignored Files: reviewboard/templates/reviews/review_detail.html
-
-
-
-
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py setup.py reviewboard/reviews/urls.py Ignored Files: reviewboard/templates/reviews/review_detail.html Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py setup.py reviewboard/reviews/urls.py Ignored Files: reviewboard/templates/reviews/review_detail.html
-
-
-
Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py setup.py reviewboard/reviews/urls.py Ignored Files: reviewboard/templates/reviews/review_detail.html Tool: Pyflakes Processed Files: reviewboard/reviews/views.py setup.py reviewboard/reviews/urls.py Ignored Files: reviewboard/templates/reviews/review_detail.html
-
-
What happens when there is one file attachment (so the 'Download File Attachments as ZIP' button is there) then delete that file? Is the button removed/disabled? I'm trying to see if there is a way to execute the code in zip_attachments on a review request without any file attachments.
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/admin/forms.py setup.py reviewboard/reviews/urls.py Ignored Files: reviewboard/templates/reviews/review_detail.html Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/admin/forms.py setup.py reviewboard/reviews/urls.py Ignored Files: reviewboard/templates/reviews/review_detail.html
-
-
-
-
-
-
-
- Testing Done:
-
Tested on:
- Linux Mint 17: Chromium 37.0.2062.120 - Linux Mint 17: Firefox 32.0.3 - Windows 8.1: Chrome 38.0.2125.101 - Windows 8.1: Firefox 32.0.3 - Windows 8.1: Internet Explorer 11 ~ Currently only working for files stored locally. Files hosted on cloud services not implemented yet.
~ Review requests with small files, large files (2mb+), and mix of both, tested and working with local and external (Amazon S3) storage options.
- Review requests with small files, large files (25mb+), and mix of both, tested and working. Dependency requirement tested by removing zipstream and running
./setup.py develop
to confirm that setup installs zipstream dependency. - Depends On:
-
- Commit:
5e3bccf1acc3135b4f64015c5e9aae68933ae061b307db01be80047410896381e5907de04073b944
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/admin/forms.py setup.py reviewboard/reviews/urls.py Ignored Files: reviewboard/templates/reviews/review_detail.html Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/admin/forms.py setup.py reviewboard/reviews/urls.py Ignored Files: reviewboard/templates/reviews/review_detail.html
-
-
-
-
-
-
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/admin/forms.py setup.py reviewboard/reviews/urls.py Ignored Files: reviewboard/templates/reviews/review_detail.html Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/admin/forms.py setup.py reviewboard/reviews/urls.py Ignored Files: reviewboard/templates/reviews/review_detail.html
-
-
-
-
-
-
-
-
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/admin/forms.py setup.py reviewboard/reviews/urls.py Ignored Files: reviewboard/templates/reviews/review_detail.html Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/admin/forms.py setup.py reviewboard/reviews/urls.py Ignored Files: reviewboard/templates/reviews/review_detail.html
-
-
-
-
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/admin/forms.py setup.py reviewboard/reviews/urls.py Ignored Files: reviewboard/templates/reviews/review_detail.html Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/admin/forms.py setup.py reviewboard/reviews/urls.py Ignored Files: reviewboard/templates/reviews/review_detail.html
-
-
- Change Summary:
-
Style fixes in code..
- Commit:
-
52e5f4353abbd51e67629f8e6aed469e5461e76d7dd103c9c456de06b5f0cda9f7310098dfdd2d47
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/admin/forms.py setup.py reviewboard/reviews/urls.py Ignored Files: reviewboard/templates/reviews/review_detail.html Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/admin/forms.py setup.py reviewboard/reviews/urls.py Ignored Files: reviewboard/templates/reviews/review_detail.html
-
-
-
-
-
You should use the 'six' version of StringIO for python 3.x compat:
from django.utils.six.moves import cStringIO as StringIO ... s = StringIO()
-
-
Since you only iterate over file_paths once, how about combining these? Then your loop belowe would look something like:
for fa in file_attachments.all(): file_dir, file_name = os.path.split(fa.file.url) ...
-
Instead of hard-coding './reviewboard/htdocs' (which won't work in a deployed server), it should use
siteconfig.get('site_media_root')
-
You're leaking the file here. This should be:
with open(file_path, 'r') as f: file_contents = f.read()
-
It's very weird to mix formatting styles here, plus you're doing two format operations where you only need one. How about just:
response['Content-Disposition'] = ( 'attachment; filename=review-attachments-%s' % review_request_id)
-
-
- Change Summary:
-
Fixed minor bugs and implemented suggestions made in previous reviews. Thanks!
- Commit:
-
7dd103c9c456de06b5f0cda9f7310098dfdd2d47cdd718da789e0990386fb63ada51baf608c99fef
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py setup.py reviewboard/reviews/urls.py Ignored Files: reviewboard/templates/reviews/review_detail.html Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py setup.py reviewboard/reviews/urls.py Ignored Files: reviewboard/templates/reviews/review_detail.html
- Change Summary:
-
Added missing file extension to review request file attachment zip.
- Commit:
-
cdd718da789e0990386fb63ada51baf608c99fefec1c6f19aa22a81de9ce5d1a6f990d41f24c5297
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py setup.py reviewboard/reviews/urls.py Ignored Files: reviewboard/templates/reviews/review_detail.html Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py setup.py reviewboard/reviews/urls.py Ignored Files: reviewboard/templates/reviews/review_detail.html
-
I have some comments about the design below. The summary is that this code doesn't actually stream zip files. Try attaching a large file (hundreds of megs or more), add some printing, open the browser's Network tab, open a process monitor to look at the dev server's memory usage, and then download. See what happens.
I know we discussed using zipstream before, but it looks like we don't actually need to add yet another dependency. Check this out and see if this solution works (once all the notes are addressed and confirmed to work): http://stackoverflow.com/a/24173992
-
Docstrings should always be in this format:
"""Single-line summary."""
or:
"""Single-line summary. Multi-line description. """
with no blank line after.
The total line length must not exceed 79 characters.
-
-
-
-
The point of the storage backends is that you should never have to do logic like this. Plus, there is absolutely no guarantee that the path from the storage backend will be a URL that you can even directly read from. Even on S3, there may be an access token required to get the URL, breaking this urlopen.
You should be able to do
fa.file.open
and read from that directly. The storage backend will handle the opening and reading.You also should not be reading the entire file in one go. The file could be 500MB in size, and you'll end up destroying the Apache process by reading it all in one go. Instead, you need to use the
chunks()
function, writing to the HTTP stream as you go.Look at the
File
documentation: https://docs.djangoproject.com/en/dev/ref/files/file/#django.core.files.File -
As above, we're not actually taking advantage of the streaming here. What you're doing is reading everything into memory and then writing it all at once, which will blow up Apache and time out the request.
Read through the docs on
StreamingHttpResponse
: https://docs.djangoproject.com/en/1.6/ref/request-response/#streaminghttpresponse-objectsNote the first bullet point. My understanding is that you just want to pass
zf
toStreamingHTTPResponse
and then begin writing the chunks to it, instead of to aStringIO
. -
"review-attachments" is a misleading prefix. This is for attachments on the review request, not reviews. However, we don't want the filename to be too long.
How about just
r%d-attachments.zip
? -
-
I know the other links are hard-coded, but we should be using
{% url download-zip-attachments review_request.display_id %}
here.
- Summary:
-
Download Review File Attachments as zipDownload Review File Attachments as tarball
- Description:
-
~ Allow users to download a zip file of all file attachments associated with a review request.
~ Button is only shown in top right of Review Request detail page when file attachments are published to the review request. ~ Download File Attachments as tarball:
~ - only working with files stored on host filesystem at the moment. + - now compresses into .tar.gz file instead. + - properly streams compressed file back to client. + - Download Attachments and Download Diff now under Downloads + menu dropdown in review request editor. ~ Hackpad - Development Log
~ https://reviewboard.hackpad.com/Andrew-Hongs-Development-Log-cXtoZbAyag7#:h=Download-All-File-Attachments- ~ - URL changed from: r/<requestID>/file/zip/ -> r/<requestID>/file/tar/
~ + [WIP] I have external storage options working but file attributes have not been fully populated.
+ Currently looking for ways to access file attributes through external storage options; ATM only found size attribute. - Testing Done:
-
Tested on:
- Linux Mint 17: Chromium 37.0.2062.120 - Linux Mint 17: Firefox 32.0.3 - Windows 8.1: Chrome 38.0.2125.101 - Windows 8.1: Firefox 32.0.3 ~ - Windows 8.1: Internet Explorer 11 ~ - Windows 8.1: Internet Explorer 11 + Tested also with curl to see if file is streaming as expected. ~ Review requests with small files, large files (2mb+), and mix of both, tested and working with local and external (Amazon S3) storage options.
~ Review requests with small files, large files (2mb+), and mix of both, tested and working with host file system storage option.
- - Dependency requirement tested by removing zipstream and running
./setup.py develop
to confirm that setup installs zipstream dependency. - Commit:
-
ec1c6f19aa22a81de9ce5d1a6f990d41f24c5297ca51f4241335c43710d752688bd87a7b44fe4df4
-
Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py setup.py reviewboard/reviews/urls.py Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/review_request_actions_primary.html reviewboard/static/rb/js/views/reviewRequestEditorView.js Tool: Pyflakes Processed Files: reviewboard/reviews/views.py setup.py reviewboard/reviews/urls.py Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/review_request_actions_primary.html reviewboard/static/rb/js/views/reviewRequestEditorView.js
-
-
- Description:
-
Download File Attachments as tarball:
- only working with files stored on host filesystem at the moment. - now compresses into .tar.gz file instead. - properly streams compressed file back to client. - Download Attachments and Download Diff now under Downloads menu dropdown in review request editor. - URL changed from: r/<requestID>/file/zip/ -> r/<requestID>/file/tar/
~ [WIP] I have external storage options working but file attributes have not been fully populated.
~ [Work in Progress]
+ I have external storage options working but file attributes have not been fully populated. Currently looking for ways to access file attributes through external storage options; ATM only found size attribute.
- Change Summary:
-
Amazon S3 support!
- Description:
-
Download File Attachments as tarball:
~ - only working with files stored on host filesystem at the moment. ~ - only working with files stored on host filesystem and Amazon S3. - now compresses into .tar.gz file instead. - properly streams compressed file back to client. - Download Attachments and Download Diff now under Downloads menu dropdown in review request editor. ~ - URL changed from: r/<requestID>/file/zip/ -> r/<requestID>/file/tar/
~ - URL changed from: r/<requestID>/file/zip/ -> r/<requestID>/file/tar/
- - [Work in Progress]
- I have external storage options working but file attributes have not been fully populated. - Currently looking for ways to access file attributes through external storage options; ATM only found size attribute. - Testing Done:
-
Tested on:
- Linux Mint 17: Chromium 37.0.2062.120 - Linux Mint 17: Firefox 32.0.3 - Windows 8.1: Chrome 38.0.2125.101 - Windows 8.1: Firefox 32.0.3 - Windows 8.1: Internet Explorer 11 Tested also with curl to see if file is streaming as expected. ~ Review requests with small files, large files (2mb+), and mix of both, tested and working with host file system storage option.
~ Review requests with small files, large files (2mb+), and mix of both, tested and working with host file system and Amazon S3 storage option.
- Commit:
-
ca51f4241335c43710d752688bd87a7b44fe4df4c20a0c6882a8e444afc7aaa444c1f735d160f107
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py setup.py reviewboard/reviews/urls.py Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/review_request_actions_primary.html reviewboard/static/rb/js/views/reviewRequestEditorView.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py setup.py reviewboard/reviews/urls.py Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/review_request_actions_primary.html reviewboard/static/rb/js/views/reviewRequestEditorView.js
-
-
-
- Change Summary:
-
Style fixes
- Commit:
-
c20a0c6882a8e444afc7aaa444c1f735d160f1079c3f62ee4ffdcd1a899ccc62fabec128d2020ed1
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py setup.py reviewboard/reviews/urls.py Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/review_request_actions_primary.html reviewboard/static/rb/js/views/reviewRequestEditorView.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py setup.py reviewboard/reviews/urls.py Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/review_request_actions_primary.html reviewboard/static/rb/js/views/reviewRequestEditorView.js
-
-
-
No spaces before/after the
"""
.This should read more like a sentence. In fact, it should really be fleshed out, to full human-readable documentation, covering what this is, what it does, how, and why.
-
I know we have some old docstrings in here, but this should be of the following format:
"""One-line summary (< 79 chars in line length). Multi-line description. """
-
This and all the other functions should go in a utility object that wraps
tarfile.TarFile
. This view should be able to very simply instantiate a thing and return the result.I'd recommend that this go in
reviewboard/attachments/streaming_tarfile.py
. -
We should just let a link be a link.
I think, given other issues reported during today's meeting, that links on the navbar are failing to just work, because we're blocking them. We should fix that instead of work around it here, since this only solves the one case.
-
- Change Summary:
-
Changes:
- Moved URL pattern to appropriate location (review_request_urls)
- Refactored streaming TarFile code into reviewboard/attachments/streaming_tarfile.py
- Docstring improvements / clean up
- Review request menu item click handler only blocks menu items with "#" as the link now. - Commit:
-
9c3f62ee4ffdcd1a899ccc62fabec128d2020ed1e131cdb07103e052d98d49e58d85638abaadeea9
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/urls.py Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/review_request_actions_primary.html reviewboard/static/rb/js/views/reviewRequestEditorView.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/urls.py Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/review_request_actions_primary.html reviewboard/static/rb/js/views/reviewRequestEditorView.js
-
- Change Summary:
-
Added back time Python module import
- Commit:
-
e131cdb07103e052d98d49e58d85638abaadeea987e2965eb2ff85f8bf9ca9ccb94c91ba262beec7
-
Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/urls.py Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/review_request_actions_primary.html reviewboard/static/rb/js/views/reviewRequestEditorView.js Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/urls.py Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/review_request_actions_primary.html reviewboard/static/rb/js/views/reviewRequestEditorView.js
-
-
-
-
-
Can you put the arguments on the first line? If they need to wrap, do:
response[...] = ('...' % out_filename)
-
Not sure we need this. The template can just check the file attachments list.
I think we just want to check
latest_file_attachments
though, notfile_attachments
. The latter contains discarded ones as well.
- Commit:
-
87e2965eb2ff85f8bf9ca9ccb94c91ba262beec72f192174beccc6419530a1e8f48093fbd305716c
- Diff:
-
Revision 18 (+178 -7)
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/attachments/streaming_tarfile.py reviewboard/reviews/urls.py Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/review_request_actions_primary.html reviewboard/static/rb/js/views/reviewRequestEditorView.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/attachments/streaming_tarfile.py reviewboard/reviews/urls.py Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/review_request_actions_primary.html reviewboard/static/rb/js/views/reviewRequestEditorView.js
-
-
-
I wasn't clear before, but what I meant in the previous review is that the templates can just check
file_attachments
, since that will evaluate to a boolean. There's no need to have a separate boolean.We have this for
has_diffs
because it's a much more complex expression not easily represented in templates. -
-
Too many backticks.
Generally, though, we don't add this sort of doc to the docstrings. This should be more higher-level, as in what the class does, as if you were explaining to someone who hasn't seen the source code but just wants a conceptual overview.
You don't need to document the arguments.
Actually, given that this is for streaming, why do we even care about the out filename? Is that even used for anything at all? I thought we weren't writing to disk.
-
No need to say all the things being outputted. Just say what the function does at a high level, and what the parameters are like. We don't need to go into the internal details.
-
-
This should be written more like how you'd say it to someone. "Build a TarInfo object representing one file in a tarball."
-
This should have a
try/except
for each thing, since a storage backend may implement one but not the other.It should also explicitly check for a
NotImplementedError
. We want to know about other exception types.No blank line before the
except
. -
-
Same comment as above.
This shouldn't know how it's going to be used (the
StreamingHttpResponse
part). Just go into the higher-level bits, like:"""Generates the tarball containing the file, in chunks. This will build the contents of the tarball, yielding chunks as they're generated. """
-
-
-
-
Will these ever be changed between instances? I can understand that it might be beneficial to keep the BLOCK_SIZE as an instance variable but would it make sense to put MODE as a class variable?
I guess part of the reason is because they are assigned constants, can't be given inputs in the init function and there are no setters for them either.
Secondly, can you maybe describe what MODE = 0644 means?