Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(2552)

Issue 4661063: Fix build configuration for benchmark zip file. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 years, 9 months ago by Siggi
Modified:
12 years, 9 months ago
CC:
sawbuck-changes_googlegroups.com
Base URL:
http://sawbuck.googlecode.com/svn/trunk/
Visibility:
Public.

Description

Fix build configuration for benchmark zip file. Committed: http://code.google.com/p/sawbuck/source/browse/#svn/trunk370

Patch Set 1 #

Total comments: 10
Unified diffs Side-by-side diffs Delta from patch set Stats (+177 lines, -100 lines) Patch
D syzygy/scripts/benchmark/benchmark.bat View 1 chunk +0 lines, -39 lines 0 comments Download
M syzygy/scripts/benchmark/benchmark.gyp View 2 chunks +15 lines, -22 lines 0 comments Download
A + syzygy/scripts/benchmark/zip_benchmark.py View 1 chunk +162 lines, -39 lines 10 comments Download

Messages

Total messages: 6
Siggi
12 years, 9 months ago (2011-06-30 20:00:13 UTC) #1
Roger McFarlane
some nits, otherwise LGTM http://codereview.appspot.com/4661063/diff/1/syzygy/scripts/benchmark/zip_benchmark.py File syzygy/scripts/benchmark/zip_benchmark.py (right): http://codereview.appspot.com/4661063/diff/1/syzygy/scripts/benchmark/zip_benchmark.py#newcode93 syzygy/scripts/benchmark/zip_benchmark.py:93: eggs = '\n'.join([' \'%s\',' % ...
12 years, 9 months ago (2011-06-30 20:23:34 UTC) #2
chrisha
lgtm... roger already took all of the good nits!
12 years, 9 months ago (2011-06-30 20:25:32 UTC) #3
Roger McFarlane
On 2011/06/30 20:25:32, chrisha wrote: > lgtm... roger already took all of the good nits! ...
12 years, 9 months ago (2011-06-30 20:26:08 UTC) #4
Siggi
I feel so ... groomed ... On Thu, Jun 30, 2011 at 4:26 PM, <rogerm@chromium.org> ...
12 years, 9 months ago (2011-06-30 20:31:31 UTC) #5
Siggi
12 years, 9 months ago (2011-06-30 20:37:54 UTC) #6
thanks, committing.

http://codereview.appspot.com/4661063/diff/1/syzygy/scripts/benchmark/zip_ben...
File syzygy/scripts/benchmark/zip_benchmark.py (right):

http://codereview.appspot.com/4661063/diff/1/syzygy/scripts/benchmark/zip_ben...
syzygy/scripts/benchmark/zip_benchmark.py:93: eggs = '\n'.join(['    \'%s\',' %
os.path.basename(egg) for egg in eggs])
On 2011/06/30 20:23:34, Roger McFarlane wrote:
> drop the escaped quotes and use %r instead of %s. This will insert the python
> representation of the string (i.e., quoted and properly escaped).

Neat, I didn't know about that. Thanks!

http://codereview.appspot.com/4661063/diff/1/syzygy/scripts/benchmark/zip_ben...
syzygy/scripts/benchmark/zip_benchmark.py:97: open(path, 'wb').write(output)
On 2011/06/30 20:23:34, Roger McFarlane wrote:
> make sure the file gets flushed/closed since you need to capture it in the zip
> later.
> 
>   with open(...) as f:
>     f.write(output)

Done.

http://codereview.appspot.com/4661063/diff/1/syzygy/scripts/benchmark/zip_ben...
syzygy/scripts/benchmark/zip_benchmark.py:112: temp_file = StringIO.StringIO()
On 2011/06/30 20:23:34, Roger McFarlane wrote:
> cStringIO.StringIO() is generally a better (more performant) choice and is a
> drop-in replacement for StringIO.StringIO if you don't need to derive from it.

Done.

http://codereview.appspot.com/4661063/diff/1/syzygy/scripts/benchmark/zip_ben...
syzygy/scripts/benchmark/zip_benchmark.py:114: zip = zipfile.ZipFile(temp_file,
'w', zipfile.ZIP_DEFLATED)
On 2011/06/30 20:23:34, Roger McFarlane wrote:
>   zip = ...
>   try:
>     for ...  
>   finally:
>     zip.close()
> 
> -- or --
> 
>   with contextlib.closing(zipfile.ZipFile(...)) as zip:
>     for ...

Done.

http://codereview.appspot.com/4661063/diff/1/syzygy/scripts/benchmark/zip_ben...
syzygy/scripts/benchmark/zip_benchmark.py:126: open(output_file,
'wb').write(output)
On 2011/06/30 20:23:34, Roger McFarlane wrote:
> Similarly:
> 
>   with open(output_file, 'wb') as stream:
>     stream.write(output)
> 
> Maybe a small helper since you do this in a couple of places.
> 
>   def _WriteFile(path, contents):
>     with open(path, 'wb') as f:
>       f.write(contents)

Done.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b