Summary

Download Review File Attachments as tarball

Review Request #6402 — Created Oct. 4, 2014 and submitted Jan. 9, 2015, 3:50 p.m.

Information

Owner

adrw.hong

Repository

Review Board

Branch

master

Bugs

Depends On

~~6432~~

Commit

2f19217...

Reviewers

Groups

reviewboard, students

People

Description

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/

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 and Amazon S3 storage option.

Issues

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

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/urls.py (Diff revision 1)

Show all issues

Col: 80
 E501 line too long (100 > 79 characters)

reviewboard/reviews/views.py (Diff revision 1)
Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```

reviewboard/reviews/views.py (Diff revision 1)

Show all issues

 local variable 'zip_path' is assigned to but never used

reviewboard/reviews/views.py (Diff revision 1)
Show all issues
```
Col: 9
 E265 block comment should start with '# '
```

reviewboard/reviews/views.py (Diff revision 1)

Show all issues

Col: 19
 E702 multiple statements on one line (semicolon)

reviewboard/reviews/views.py (Diff revision 1)
Show all issues
```
Col: 36
 E703 statement ends with a semicolon
```
reviewboard/reviews/views.py (Diff revision 1)
Show all issues
```
Col: 9
 E265 block comment should start with '# '
```

reviewboard/reviews/views.py (Diff revision 1)

Show all issues

 local variable 'zip_filename' is assigned to but never used

reviewboard/reviews/views.py (Diff revision 1)
Show all issues
```
Col: 80
 E501 line too long (82 > 79 characters)
```

reviewboard/reviews/views.py (Diff revision 1)

Show all issues

 list comprehension redefines 'file_attachment' from line 621

Commit:

-	c1fa19bc15b403178392e24318ae7f3df86709f0
+	39271c9bf046f4f28354651ce0b1a8a7166f9cfe

Diff:

Revision 2 (+48 -2)

Show changes

	setup.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	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/urls.py (Diff revision 2)
Show all issues
```
Col: 49
 W291 trailing whitespace
```
reviewboard/reviews/urls.py (Diff revision 2)
Show all issues
```
Col: 27
 W291 trailing whitespace
```
reviewboard/reviews/views.py (Diff revision 2)
Show all issues
```
Col: 38
 W291 trailing whitespace
```
reviewboard/reviews/views.py (Diff revision 2)
Show all issues
```
Col: 9
 E113 unexpected indentation
```

Commit:

-	39271c9bf046f4f28354651ce0b1a8a7166f9cfe
+	04f43e506b27b7275d02bd501749fa75e68102e5

Diff:

Revision 3 (+47 -2)

Show changes

	setup.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	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

reviewboard/reviews/views.py (Diff revision 3)
Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```

reviewboard/reviews/views.py (Diff revision 3)

Show all issues

 list comprehension redefines 'file_attachment' from line 618

Commit:

-	04f43e506b27b7275d02bd501749fa75e68102e5
+	f9402f1bfef86c4f60b758379e78d8ae10229f9b

Diff:

Revision 4 (+48 -2)

Show changes

	setup.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	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

reviewboard/reviews/views.py (Diff revision 4)

Show all issues

 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.

adrw.hong Oct. 5, 2014, 7:29 a.m.

When you delete and haven't published it, then there is a blank ZIP file download. I'm looking into preventing that from happening.
The button is removed when that change is published or when there are no file attachments present though.

reviewboard/reviews/views.py (Diff revision 4)

Show all issues

There should be a single blank line before a block

```
Other than that, it looks good!
```

Commit:

-	f9402f1bfef86c4f60b758379e78d8ae10229f9b
+	5e3bccf1acc3135b4f64015c5e9aae68933ae061

Diff:

Revision 5 (+61 -2)

Show changes

	setup.py
	reviewboard/admin/forms.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	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

reviewboard/reviews/views.py (Diff revision 5)
Show all issues
```
Col: 5
 E303 too many blank lines (2)
```

reviewboard/reviews/views.py (Diff revision 5)

Show all issues

Col: 6
 E111 indentation is not a multiple of four

reviewboard/reviews/views.py (Diff revision 5)
Show all issues
```
Col: 6
 E113 unexpected indentation
```
reviewboard/reviews/views.py (Diff revision 5)
Show all issues
```
Col: 6
 E265 block comment should start with '# '
```
reviewboard/reviews/views.py (Diff revision 5)
Show all issues
```
Col: 6
 E303 too many blank lines (2)
```
reviewboard/reviews/views.py (Diff revision 5)
Show all issues
```
Col: 9
 E265 block comment should start with '# '
```

reviewboard/reviews/views.py (Diff revision 5)

Show all issues

 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?

adrw.hong Oct. 16, 2014, 7:44 a.m.

I was playing around with different implementations to see if it makes any difference.
There was a function, I forget which, that took a file-like object as a parameter instead of bytes.

reviewboard/admin/forms.py (Diff revision 5)

Show all issues

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.

adrw.hong Oct. 16, 2014, 9:17 a.m.

Would editting it in the review request page achieve the same thing?

brennie Oct. 16, 2014, 9:31 a.m.

You should rebase your development branch on an updated master since 6432 has been submitted. Then you can re-run rbt post and the change should not appear. Alternatively, you can run rbt post -X reviewboard/admin/forms.py

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:

+	~~6432 - Attachment Storage Options (admin) not saving -- always reverts back to local storage option~~

Commit:

-	5e3bccf1acc3135b4f64015c5e9aae68933ae061
+	b307db01be80047410896381e5907de04073b944

Diff:

Revision 6 (+66 -2)

Show changes

	setup.py
	reviewboard/admin/forms.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	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

reviewboard/reviews/views.py (Diff revision 6)
Show all issues
```
Col: 80
 E501 line too long (82 > 79 characters)
```

reviewboard/reviews/views.py (Diff revision 6)

Show all issues

Col: 21
 E128 continuation line under-indented for visual indent

reviewboard/reviews/views.py (Diff revision 6)

Show all issues

Col: 21
 E128 continuation line under-indented for visual indent

reviewboard/reviews/views.py (Diff revision 6)
Show all issues
```
Col: 5
 E303 too many blank lines (2)
```
reviewboard/reviews/views.py (Diff revision 6)
Show all issues
```
Col: 80
 E501 line too long (87 > 79 characters)
```

reviewboard/reviews/views.py (Diff revision 6)

Show all issues

 list comprehension redefines 'file_attachment' from line 634

Commit:

-	b307db01be80047410896381e5907de04073b944
+	8c42405861c3d230cf074a7ce0d1e413f3787d13

Diff:

Revision 7 (+67 -2)

Show changes

	setup.py
	reviewboard/admin/forms.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	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

reviewboard/reviews/views.py (Diff revision 7)
Show all issues
```
Col: 40
 W291 trailing whitespace
```

reviewboard/reviews/views.py (Diff revision 7)

Show all issues

Col: 9
 E128 continuation line under-indented for visual indent

reviewboard/reviews/views.py (Diff revision 7)

Show all issues

Col: 56
 E502 the backslash is redundant between brackets

reviewboard/reviews/views.py (Diff revision 7)

Show all issues

Col: 72
 E502 the backslash is redundant between brackets

reviewboard/reviews/views.py (Diff revision 7)

Show all issues

Col: 21
 E128 continuation line under-indented for visual indent

reviewboard/reviews/views.py (Diff revision 7)

Show all issues

Col: 21
 E128 continuation line under-indented for visual indent

reviewboard/reviews/views.py (Diff revision 7)

Show all issues

Col: 13
 E128 continuation line under-indented for visual indent

reviewboard/reviews/views.py (Diff revision 7)

Show all issues

 list comprehension redefines 'file_attachment' from line 635

Commit:

-	8c42405861c3d230cf074a7ce0d1e413f3787d13
+	e338a791dded0433f9c33e5a26a0f37eef8f7e8f

Diff:

Revision 8 (+68 -2)

Show changes

	setup.py
	reviewboard/admin/forms.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	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

reviewboard/reviews/views.py (Diff revision 8)

Show all issues

Col: 41
 E502 the backslash is redundant between brackets

reviewboard/reviews/views.py (Diff revision 8)

Show all issues

Col: 9
 E128 continuation line under-indented for visual indent

reviewboard/reviews/views.py (Diff revision 8)

Show all issues

Col: 13
 E128 continuation line under-indented for visual indent

reviewboard/reviews/views.py (Diff revision 8)

Show all issues

 list comprehension redefines 'file_attachment' from line 636

Commit:

-	e338a791dded0433f9c33e5a26a0f37eef8f7e8f
+	52e5f4353abbd51e67629f8e6aed469e5461e76d

Diff:

Revision 9 (+68 -2)

Show changes

	setup.py
	reviewboard/admin/forms.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	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

reviewboard/reviews/views.py (Diff revision 9)
Show all issues
```
 undefined name 'eview_request_id'
```

reviewboard/reviews/views.py (Diff revision 9)

Show all issues

 list comprehension redefines 'file_attachment' from line 636

Change Summary:

Style fixes in code..

Commit:

-	52e5f4353abbd51e67629f8e6aed469e5461e76d
+	7dd103c9c456de06b5f0cda9f7310098dfdd2d47

Diff:

Revision 10 (+68 -2)

Show changes

	setup.py
	reviewboard/admin/forms.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	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

reviewboard/reviews/views.py (Diff revision 10)

Show all issues

 list comprehension redefines 'file_attachment' from line 636

Change Summary:

Out of WIP!

Summary:

-	[WIP] Download Review File Attachments as zip
+	Download Review File Attachments as zip

reviewboard/reviews/views.py (Diff revision 10)
Show all issues
```
You should use os.path.join to create file paths.
```

reviewboard/reviews/views.py (Diff revision 10)

Show all issues

Suggestion: why not bool(file_attachments)?

Empty lists will evaluate to false and so will None.

reviewboard/admin/forms.py (Diff revision 10)

Show all issues

As mentioned earlier, can you pull the latest master and rebase?

reviewboard/reviews/views.py (Diff revision 10)
Show all issues
```
One too many blank lines here.
```

reviewboard/reviews/views.py (Diff revision 10)

Show all issues

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)
Show all issues
```
Revert this change.
```

reviewboard/reviews/views.py (Diff revision 10)

Show all issues

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)

Show all issues

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)

Show all issues

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)

Show all issues

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)

Show all issues

Can we make the URL /file/zip/ ? That way it fits in with the existing /file/N/ URLs.

setup.py (Diff revision 10)

Show all issues

Is this necessary? Anything on PyPI should automatically be included...

Change Summary:

Fixed minor bugs and implemented suggestions made in previous reviews. Thanks!

Commit:

-	7dd103c9c456de06b5f0cda9f7310098dfdd2d47
+	cdd718da789e0990386fb63ada51baf608c99fef

Diff:

Revision 11 (+71 -3)

Show changes

	setup.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	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

reviewboard/reviews/views.py (Diff revision 11)

Show all issues

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)

AD

adrw.hong Oct. 25, 2014, 11:07 a.m.
```
EDIT: removed -- wrong comment.
```

reviewboard/reviews/views.py (Diff revision 11)

Show all issues

Shouldn't this have a .zip extension?

adrw.hong Oct. 25, 2014, 11:07 a.m.

Good catch! Thanks for pointing that out. The browser automatically knew it was a zip so I assumed it was all good there.

Change Summary:

Added missing file extension to review request file attachment zip.

Commit:

-	cdd718da789e0990386fb63ada51baf608c99fef
+	ec1c6f19aa22a81de9ce5d1a6f990d41f24c5297

Diff:

Revision 12 (+71 -3)

Show changes

	setup.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	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

Ship it!

setup.py (Diff revision 12)

Show all issues

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

chipx86 Nov. 2, 2014, 11:34 a.m.

Okay, I've looked into this further, and now I understand better. There's no way to chunk-write to a ZipFile instance without rewriting part of ZipFile itself.

Given that, ZipStream does do what we want, and does it how we'd do it, so that's worth using.

reviewboard/reviews/views.py (Diff revision 12)

Show all issues

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)

Show all issues

I'd just put .all() at the end of this one. No point doing it below.

reviewboard/reviews/views.py (Diff revision 12)
Show all issues
```
Blank line between these.
```
reviewboard/reviews/views.py (Diff revision 12)
Show all issues
```
Blank line between these.
```

reviewboard/reviews/views.py (Diff revision 12)

Show all issues

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)

Show all issues

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-objects

Note the first bullet point. My understanding is that you just want to pass zf to StreamingHTTPResponse and then begin writing the chunks to it, instead of to a StringIO.

reviewboard/reviews/views.py (Diff revision 12)

Show all issues

"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)

Show all issues

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)

Show all issues

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 zip
+	Download 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:

-	ec1c6f19aa22a81de9ce5d1a6f990d41f24c5297
+	ca51f4241335c43710d752688bd87a7b44fe4df4

Diff:

Revision 13 (+166 -7)

Show changes

	setup.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/js/views/reviewRequestEditorView.js
	reviewboard/templates/reviews/review_detail.html
	reviewboard/templates/reviews/review_request_actions_primary.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/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)

Show all issues

 local variable 'total_size' is assigned to but never used

reviewboard/reviews/views.py (Diff revision 13)
Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```

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.

setup.py (Diff revision 13)

Show all issues

If you're only compressing as tar.gz you won't need zipstream.

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:

-	ca51f4241335c43710d752688bd87a7b44fe4df4
+	c20a0c6882a8e444afc7aaa444c1f735d160f107

Diff:

Revision 14 (+160 -7)

Show changes

	setup.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/js/views/reviewRequestEditorView.js
	reviewboard/templates/reviews/review_detail.html
	reviewboard/templates/reviews/review_request_actions_primary.html

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)
Show all issues
```
 'os' imported but unused
```

reviewboard/reviews/views.py (Diff revision 14)

Show all issues

 local variable 'siteconfig' is assigned to but never used

reviewboard/reviews/views.py (Diff revision 14)
Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```

Change Summary:

Style fixes

Commit:

-	c20a0c6882a8e444afc7aaa444c1f735d160f107
+	9c3f62ee4ffdcd1a899ccc62fabec128d2020ed1

Diff:

Revision 15 (+159 -7)

Show changes

	setup.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/js/views/reviewRequestEditorView.js
	reviewboard/templates/reviews/review_detail.html
	reviewboard/templates/reviews/review_request_actions_primary.html

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/urls.py (Diff revision 15)
Show all issues
```
This should go under review_request_urls.
```

reviewboard/reviews/views.py (Diff revision 15)

Show all issues

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)

Show all issues

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)

Show all issues

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)

Show all issues

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.

setup.py (Diff revision 15)

Show all issues

These don't look like they were meant to be in this change?

reviewboard/reviews/views.py (Diff revision 15)

Show all issues

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:

-	9c3f62ee4ffdcd1a899ccc62fabec128d2020ed1
+	e131cdb07103e052d98d49e58d85638abaadeea9

Diff:

Revision 16 (+61 -8)

Show changes

	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/js/views/reviewRequestEditorView.js
	reviewboard/templates/reviews/review_detail.html
	reviewboard/templates/reviews/review_request_actions_primary.html

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

reviewboard/reviews/views.py (Diff revision 16)
Show all issues
```
 undefined name 'time'
```

Change Summary:

Added back time Python module import

Commit:

-	e131cdb07103e052d98d49e58d85638abaadeea9
+	87e2965eb2ff85f8bf9ca9ccb94c91ba262beec7

Diff:

Revision 17 (+61 -7)

Show changes

	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/js/views/reviewRequestEditorView.js
	reviewboard/templates/reviews/review_detail.html
	reviewboard/templates/reviews/review_request_actions_primary.html

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)

Show all issues

This must be added alphabetically to the list of imports (after reviewboard.attachments.models.)

reviewboard/reviews/views.py (Diff revision 17)

Show all issues

How about "Downloads all file attachments on a review request as a tarball." ?

reviewboard/reviews/views.py (Diff revision 17)

Show all issues

"... are downloaded as a tarball."

Not sure we need to talk about what isn't supported.

reviewboard/reviews/views.py (Diff revision 17)

Show all issues

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)

Show all issues

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, not file_attachments. The latter contains discarded ones as well.

Looks like this is missing the implementation of the streaming tar class.

Commit:

-	87e2965eb2ff85f8bf9ca9ccb94c91ba262beec7
+	2f192174beccc6419530a1e8f48093fbd305716c

Diff:

Revision 18 (+178 -7)

Show changes

	reviewboard/attachments/streaming_tarfile.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/js/views/reviewRequestEditorView.js
	reviewboard/templates/reviews/review_detail.html
	reviewboard/templates/reviews/review_request_actions_primary.html

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 revision 18)
Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```

reviewboard/reviews/views.py (Diff revisions 17 - 18)

Show all issues

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)
Show all issues
```
No space after """.
```

reviewboard/attachments/streaming_tarfile.py (Diff revision 18)

Show all issues

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)

Show all issues

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)
Show all issues
```
These should be class-level constants.
```

reviewboard/attachments/streaming_tarfile.py (Diff revision 18)

Show all issues

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)

Show all issues

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)
Show all issues
```
We should yield before we close.
```

reviewboard/attachments/streaming_tarfile.py (Diff revision 18)

Show all issues

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)
Show all issues
```
Blank line between these.
```
reviewboard/attachments/streaming_tarfile.py (Diff revision 18)
Show all issues
```
yield isn't a function, so you shouldn't use parens.
```

reviewboard/attachments/streaming_tarfile.py (Diff revision 18)

Show all issues

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?

Status: Closed (submitted)

Change Summary:

Pushed to ucosp/adrw.hong/download-attachments (b133167)