Chromium Code Reviews
Help | Chromium Project | Sign in
(18)

Issue 9638020: Refactor isolate.py to be more functional and extensible (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 1 month ago by M-A Ruel
Modified:
2 years, 1 month ago
Reviewers:
csharp
CC:
chromium-reviews_chromium.org, pam+watch_chromium.org, Roger Tawa
Visibility:
Public.

Description

Refactor isolate.py to be more functional and extensible

- Works around poor handling of -- in optparse.
- Reuse sys.executable for python scripts.
- Moved all file handling code into tree_creator.py (which is a bit illnamed).
- Fixed hashtable mode to work without --outdir.
- Properly save the files mode.
- Properly save the file hash, only when needed.
- Always save the result file in json.

TBR=csharp@chromium.org
BUG=98640
TEST='GYP_DEFINES="$GYP_DEFINES tests_run=hashtable" build/gyp_chromium' works

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=125784

Patch Set 1 #

Patch Set 2 : Rewrote most of the script #

Patch Set 3 : More extensive testing, saner mode saving, etc #

Total comments: 6
Unified diffs Side-by-side diffs Delta from patch set Stats (+232 lines, -134 lines) Lint Patch
A tools/isolate/data/test_file1.txt View 1 2 1 chunk +1 line, -0 lines 0 comments ? errors Download
A tools/isolate/data/test_file2.txt View 1 2 1 chunk +1 line, -0 lines 0 comments ? errors Download
M tools/isolate/isolate.py View 1 2 6 chunks +95 lines, -93 lines 4 comments ? errors Download
M tools/isolate/isolate_test.py View 1 2 9 chunks +73 lines, -20 lines 2 comments ? errors Download
M tools/isolate/tree_creator.py View 1 2 6 chunks +62 lines, -21 lines 0 comments ? errors Download
Commit:

Messages

Total messages: 8
M-A Ruel
Dependent on http://codereview.chromium.org/9621014/
2 years, 1 month ago #1
M-A Ruel
GYP_DEFINES=tests_run=hashtable now archives in the output directory. Technically it'd be better to save elsewhere but ...
2 years, 1 month ago #2
csharp
https://chromiumcodereview.appspot.com/9638020/diff/6001/tools/isolate/isolate.py File tools/isolate/isolate.py (right): https://chromiumcodereview.appspot.com/9638020/diff/6001/tools/isolate/isolate.py#newcode51 tools/isolate/isolate.py:51: if '--' in args: Will optparse always be messing ...
2 years, 1 month ago #3
M-A Ruel
https://chromiumcodereview.appspot.com/9638020/diff/6001/tools/isolate/isolate.py File tools/isolate/isolate.py (right): https://chromiumcodereview.appspot.com/9638020/diff/6001/tools/isolate/isolate.py#newcode51 tools/isolate/isolate.py:51: if '--' in args: On 2012/03/08 21:53:39, csharp wrote: ...
2 years, 1 month ago #4
csharp
lgtm
2 years, 1 month ago #5
I haz the power (commit-bot)
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an ...
2 years, 1 month ago #6
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/maruel@chromium.org/9638020/6001
2 years, 1 month ago #7
I haz the power (commit-bot)
2 years, 1 month ago #8
Change committed as 125784
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6