-
-
-
-
reviewboard/reviews/views.py (Diff revision 1) local variable 'zip_path' is assigned to but never used
-
-
reviewboard/reviews/views.py (Diff revision 1) Col: 19 E702 multiple statements on one line (semicolon)
-
-
-
reviewboard/reviews/views.py (Diff revision 1) local variable 'zip_filename' is assigned to but never used
-
-
reviewboard/reviews/views.py (Diff revision 1) list comprehension redefines 'file_attachment' from line 621
Download Review File Attachments as tarball
Review Request #6402 — Created Oct. 4, 2014 and submitted
Information | |
---|---|
adrw.hong | |
Review Board | |
master | |
|
|
2f19217... | |
Reviewers | |
reviewboard, students | |
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) |
![]() |
|
Col: 1 E302 expected 2 blank lines, found 1 |
![]() |
|
local variable 'zip_path' is assigned to but never used |
![]() |
|
Col: 9 E265 block comment should start with '# ' |
![]() |
|
Col: 19 E702 multiple statements on one line (semicolon) |
![]() |
|
Col: 36 E703 statement ends with a semicolon |
![]() |
|
Col: 9 E265 block comment should start with '# ' |
![]() |
|
local variable 'zip_filename' is assigned to but never used |
![]() |
|
Col: 80 E501 line too long (82 > 79 characters) |
![]() |
|
list comprehension redefines 'file_attachment' from line 621 |
![]() |
|
Col: 49 W291 trailing whitespace |
![]() |
|
Col: 27 W291 trailing whitespace |
![]() |
|
Col: 38 W291 trailing whitespace |
![]() |
|
Col: 9 E113 unexpected indentation |
![]() |
|
Col: 80 E501 line too long (80 > 79 characters) |
![]() |
|
list comprehension redefines 'file_attachment' from line 618 |
![]() |
|
There should be a single blank line before a block |
|
|
list comprehension redefines 'file_attachment' from line 619 |
![]() |
|
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 … |
|
|
Col: 5 E303 too many blank lines (2) |
![]() |
|
Col: 6 E111 indentation is not a multiple of four |
![]() |
|
Col: 6 E113 unexpected indentation |
![]() |
|
Col: 6 E265 block comment should start with '# ' |
![]() |
|
Col: 6 E303 too many blank lines (2) |
![]() |
|
Col: 9 E265 block comment should start with '# ' |
![]() |
|
list comprehension redefines 'file_attachment' from line 629 |
![]() |
|
Col: 80 E501 line too long (82 > 79 characters) |
![]() |
|
Col: 21 E128 continuation line under-indented for visual indent |
![]() |
|
Col: 21 E128 continuation line under-indented for visual indent |
![]() |
|
Col: 5 E303 too many blank lines (2) |
![]() |
|
Col: 80 E501 line too long (87 > 79 characters) |
![]() |
|
list comprehension redefines 'file_attachment' from line 634 |
![]() |
|
Col: 40 W291 trailing whitespace |
![]() |
|
Col: 9 E128 continuation line under-indented for visual indent |
![]() |
|
Col: 56 E502 the backslash is redundant between brackets |
![]() |
|
Col: 72 E502 the backslash is redundant between brackets |
![]() |
|
Col: 21 E128 continuation line under-indented for visual indent |
![]() |
|
Col: 21 E128 continuation line under-indented for visual indent |
![]() |
|
Col: 13 E128 continuation line under-indented for visual indent |
![]() |
|
list comprehension redefines 'file_attachment' from line 635 |
![]() |
|
Col: 41 E502 the backslash is redundant between brackets |
![]() |
|
Col: 9 E128 continuation line under-indented for visual indent |
![]() |
|
Col: 13 E128 continuation line under-indented for visual indent |
![]() |
|
list comprehension redefines 'file_attachment' from line 636 |
![]() |
|
undefined name 'eview_request_id' |
![]() |
|
list comprehension redefines 'file_attachment' from line 636 |
![]() |
|
As mentioned earlier, can you pull the latest master and rebase? |
|
|
One too many blank lines here. |
|
|
You should use the 'six' version of StringIO for python 3.x compat: from django.utils.six.moves import cStringIO as StringIO ... s … |
|
|
Revert this change. |
|
|
Since you only iterate over file_paths once, how about combining these? Then your loop belowe would look something like: for … |
|
|
Instead of hard-coding './reviewboard/htdocs' (which won't work in a deployed server), it should use siteconfig.get('site_media_root') |
|
|
You should use os.path.join to create file paths. |
|
|
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 … |
|
|
Suggestion: why not bool(file_attachments)? Empty lists will evaluate to false and so will None. |
|
|
list comprehension redefines 'file_attachment' from line 636 |
![]() |
|
Can we make the URL /file/zip/ ? That way it fits in with the existing /file/N/ URLs. |
|
|
Is this necessary? Anything on PyPI should automatically be included... |
|
|
Why not wrap it in () so that you don't need the \\, i.e. response['Content-Disposition'] = ( 'attachment; filename=review-attachments-%s' % … |
|
|
Shouldn't this have a .zip extension? |
|
|
Docstrings should always be in this format: """Single-line summary.""" or: """Single-line summary. Multi-line description. """ with no blank line after. … |
|
|
I'd just put .all() at the end of this one. No point doing it below. |
|
|
Blank line between these. |
|
|
Blank line between these. |
|
|
The point of the storage backends is that you should never have to do logic like this. Plus, there is … |
|
|
As above, we're not actually taking advantage of the streaming here. What you're doing is reading everything into memory and … |
|
|
"review-attachments" is a misleading prefix. This is for attachments on the review request, not reviews. However, we don't want the … |
|
|
We should probably have both of these within a "Download" menu, like we have with "Update." |
|
|
I know the other links are hard-coded, but we should be using {% url download-zip-attachments review_request.display_id %} here. |
|
|
Should have a comma at end of line. That way future dependancies will only be a 1-line change. |
|
|
local variable 'total_size' is assigned to but never used |
![]() |
|
Col: 80 E501 line too long (80 > 79 characters) |
![]() |
|
If you're only compressing as tar.gz you won't need zipstream. |
|
|
'os' imported but unused |
![]() |
|
local variable 'siteconfig' is assigned to but never used |
![]() |
|
Col: 80 E501 line too long (80 > 79 characters) |
![]() |
|
This should go under review_request_urls. |
|
|
No spaces before/after the """. This should read more like a sentence. In fact, it should really be fleshed out, … |
|
|
I know we have some old docstrings in here, but this should be of the following format: """One-line summary (< … |
|
|
This and all the other functions should go in a utility object that wraps tarfile.TarFile. This view should be able … |
|
|
Docstrings shouldn't have a space after the first triple quote, e.g. """Build TarInfo...""" Same below |
|
|
We should just let a link be a link. I think, given other issues reported during today's meeting, that links … |
|
|
These don't look like they were meant to be in this change? |
|
|
undefined name 'time' |
![]() |
|
This must be added alphabetically to the list of imports (after reviewboard.attachments.models.) |
|
|
How about "Downloads all file attachments on a review request as a tarball." ? |
|
|
"... are downloaded as a tarball." Not sure we need to talk about what isn't supported. |
|
|
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 … |
|
|
I wasn't clear before, but what I meant in the previous review is that the templates can just check file_attachments, … |
|
|
No space after """. |
|
|
Too many backticks. Generally, though, we don't add this sort of doc to the docstrings. This should be more higher-level, … |
|
|
No need to say all the things being outputted. Just say what the function does at a high level, and … |
|
|
These should be class-level constants. |
|
|
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 … |
|
|
This should have a try/except for each thing, since a storage backend may implement one but not the other. It … |
|
|
We should yield before we close. |
|
|
Same comment as above. This shouldn't know how it's going to be used (the StreamingHttpResponse part). Just go into the … |
|
|
Blank line between these. |
|
|
yield isn't a function, so you shouldn't use parens. |
|
|
Col: 80 E501 line too long (80 > 79 characters) |
![]() |

Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+48 -2) |

-
Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py setup.py reviewboard/reviews/urls.py Ignored Files: reviewboard/templates/reviews/review_detail.html
-
-
-
-
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+47 -2) |

-
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
-
-
reviewboard/reviews/views.py (Diff revision 3) list comprehension redefines 'file_attachment' from line 618
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+48 -2) |

-
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
-
reviewboard/reviews/views.py (Diff revision 4) list comprehension redefines 'file_attachment' from line 619
-
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.
-
-
-
Other than that, it looks good!
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+61 -2) |

-
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
-
-
-
-
-
-
-
reviewboard/reviews/views.py (Diff revision 5) list comprehension redefines 'file_attachment' from line 629
-
-
reviewboard/reviews/views.py (Diff revisions 4 - 5) Out of curiosity, why the round-trip through the StringIO?
-
-
reviewboard/admin/forms.py (Diff revision 5) 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 off of that diff via:
rbt diff dev/fix-storage-saving..dev/zip-attachments
but using the correct branch names.
Testing Done: |
|
||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Depends On: |
|
||||||||||||||||||||||||||||||||||||
Commit: |
|
||||||||||||||||||||||||||||||||||||
Diff: |
Revision 6 (+66 -2) |

-
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
-
-
reviewboard/reviews/views.py (Diff revision 6) Col: 21 E128 continuation line under-indented for visual indent
-
reviewboard/reviews/views.py (Diff revision 6) Col: 21 E128 continuation line under-indented for visual indent
-
-
-
reviewboard/reviews/views.py (Diff revision 6) list comprehension redefines 'file_attachment' from line 634
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+67 -2) |

-
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
-
-
reviewboard/reviews/views.py (Diff revision 7) Col: 9 E128 continuation line under-indented for visual indent
-
reviewboard/reviews/views.py (Diff revision 7) Col: 56 E502 the backslash is redundant between brackets
-
reviewboard/reviews/views.py (Diff revision 7) Col: 72 E502 the backslash is redundant between brackets
-
reviewboard/reviews/views.py (Diff revision 7) Col: 21 E128 continuation line under-indented for visual indent
-
reviewboard/reviews/views.py (Diff revision 7) Col: 21 E128 continuation line under-indented for visual indent
-
reviewboard/reviews/views.py (Diff revision 7) Col: 13 E128 continuation line under-indented for visual indent
-
reviewboard/reviews/views.py (Diff revision 7) list comprehension redefines 'file_attachment' from line 635
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 8 (+68 -2) |

-
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
-
reviewboard/reviews/views.py (Diff revision 8) Col: 41 E502 the backslash is redundant between brackets
-
reviewboard/reviews/views.py (Diff revision 8) Col: 9 E128 continuation line under-indented for visual indent
-
reviewboard/reviews/views.py (Diff revision 8) Col: 13 E128 continuation line under-indented for visual indent
-
reviewboard/reviews/views.py (Diff revision 8) list comprehension redefines 'file_attachment' from line 636
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 9 (+68 -2) |

-
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
-
-
reviewboard/reviews/views.py (Diff revision 9) list comprehension redefines 'file_attachment' from line 636
Change Summary:
Style fixes in code..
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 10 (+68 -2) |

-
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
-
reviewboard/reviews/views.py (Diff revision 10) list comprehension redefines 'file_attachment' from line 636
-
-
-
reviewboard/reviews/views.py (Diff revision 10) Suggestion: why not
bool(file_attachments)
?Empty lists will evaluate to false and so will
None
.
-
-
reviewboard/admin/forms.py (Diff revision 10) As mentioned earlier, can you pull the latest master and rebase?
-
-
reviewboard/reviews/views.py (Diff revision 10) You should use the 'six' version of StringIO for python 3.x compat:
from django.utils.six.moves import cStringIO as StringIO ... s = StringIO()
-
-
reviewboard/reviews/views.py (Diff revision 10) 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) ...
-
reviewboard/reviews/views.py (Diff revision 10) Instead of hard-coding './reviewboard/htdocs' (which won't work in a deployed server), it should use
siteconfig.get('site_media_root')
-
reviewboard/reviews/views.py (Diff revision 10) You're leaking the file here. This should be:
with open(file_path, 'r') as f: file_contents = f.read()
-
reviewboard/reviews/views.py (Diff revision 10) 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)
-
reviewboard/templates/reviews/review_detail.html (Diff revision 10) Can we make the URL /file/zip/ ? That way it fits in with the existing /file/N/ URLs.
-
Change Summary:
Fixed minor bugs and implemented suggestions made in previous reviews. Thanks!
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 11 (+71 -3) |

-
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
-
-
reviewboard/reviews/views.py (Diff revision 11) Why not wrap it in () so that you don't need the
\\
, i.e.response['Content-Disposition'] = ( 'attachment; filename=review-attachments-%s' % review_request_id)
-
Change Summary:
Added missing file extension to review request file attachment zip.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 12 (+71 -3) |

-
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
-
-
setup.py (Diff revision 12) Should have a comma at end of line. That way future dependancies will only be a 1-line change.
-
-
reviewboard/reviews/views.py (Diff revision 12) I am not too familiar with how python handles file IO, but shouldn't the file be read / written in chunks? What happens if the file is really big (many gbs), will this put all of it into memory?
-
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
-
reviewboard/reviews/views.py (Diff revision 12) 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.
-
reviewboard/reviews/views.py (Diff revision 12) I'd just put
.all()
at the end of this one. No point doing it below. -
-
-
reviewboard/reviews/views.py (Diff revision 12) 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 -
reviewboard/reviews/views.py (Diff revision 12) 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
. -
reviewboard/reviews/views.py (Diff revision 12) "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
? -
reviewboard/templates/reviews/review_detail.html (Diff revision 12) We should probably have both of these within a "Download" menu, like we have with "Update."
-
reviewboard/templates/reviews/review_detail.html (Diff revision 12) I know the other links are hard-coded, but we should be using
{% url download-zip-attachments review_request.display_id %}
here.
Summary: |
|
|||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||
Testing Done: |
|
|||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
|||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 13 (+166 -7) |

-
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
-
reviewboard/reviews/views.py (Diff revision 13) local variable 'total_size' is assigned to but never used
-
Description: |
|
---|
Change Summary:
Amazon S3 support!
Description: |
|
||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 14 (+160 -7) |

-
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
-
-
reviewboard/reviews/views.py (Diff revision 14) local variable 'siteconfig' is assigned to but never used
-
Change Summary:
Style fixes
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 15 (+159 -7) |

-
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
-
-
-
reviewboard/reviews/views.py (Diff revision 15) 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.
-
reviewboard/reviews/views.py (Diff revision 15) 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. """
-
reviewboard/reviews/views.py (Diff revision 15) 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
. -
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 15) 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.
-
-
-
reviewboard/reviews/views.py (Diff revision 15) Docstrings shouldn't have a space after the first triple quote, e.g.
"""Build TarInfo..."""
Same below
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: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 16 (+61 -8) |

-
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: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 17 (+61 -7) |

-
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
-
-
reviewboard/reviews/views.py (Diff revision 17) This must be added alphabetically to the list of imports (after
reviewboard.attachments.models
.) -
reviewboard/reviews/views.py (Diff revision 17) How about "Downloads all file attachments on a review request as a tarball." ?
-
reviewboard/reviews/views.py (Diff revision 17) "... are downloaded as a tarball."
Not sure we need to talk about what isn't supported.
-
reviewboard/reviews/views.py (Diff revision 17) Can you put the arguments on the first line? If they need to wrap, do:
response[...] = ('...' % out_filename)
-
reviewboard/reviews/views.py (Diff revision 17) 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: |
|
||||
---|---|---|---|---|---|
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
-
-
-
reviewboard/reviews/views.py (Diff revisions 17 - 18) 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. -
-
reviewboard/attachments/streaming_tarfile.py (Diff revision 18) 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.
-
reviewboard/attachments/streaming_tarfile.py (Diff revision 18) 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.
-
reviewboard/attachments/streaming_tarfile.py (Diff revision 18) These should be class-level constants.
-
reviewboard/attachments/streaming_tarfile.py (Diff revision 18) This should be written more like how you'd say it to someone. "Build a TarInfo object representing one file in a tarball."
-
reviewboard/attachments/streaming_tarfile.py (Diff revision 18) 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
. -
-
reviewboard/attachments/streaming_tarfile.py (Diff revision 18) 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. """
-
-
reviewboard/attachments/streaming_tarfile.py (Diff revision 18) yield
isn't a function, so you shouldn't use parens.
-
-
reviewboard/attachments/streaming_tarfile.py (Diff revision 18) 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?