Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1213613011:
Allow move_source_file.py and sort-headers.py to work on blink. (Closed)
DescriptionAllow move_source_file.py and sort-headers.py to work on blink.
BUG=507539
Committed: https://crrev.com/5c5d6f9969910908c0bb4e8521077ee8c9d27c16
Cr-Commit-Position: refs/heads/master@{#338824}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Feedback #
Total comments: 6
Patch Set 3 : More #
Total comments: 4
Patch Set 4 : Feedback #Messages
Total messages: 18 (5 generated)
benwells@chromium.org changed reviewers: + mgiuca@chromium.org
mgiuca, who knows python, for initial review.
https://codereview.chromium.org/1213613011/diff/1/tools/git/move_source_file.py File tools/git/move_source_file.py (right): https://codereview.chromium.org/1213613011/diff/1/tools/git/move_source_file.... tools/git/move_source_file.py:91: def UpdateIncludePathForBlink(path): This should be moved to the top level. (Only make a nested function if you need to close over local variables.) Maybe add a docstring as well to explain why. https://codereview.chromium.org/1213613011/diff/1/tools/git/move_source_file.... tools/git/move_source_file.py:92: for prefix in ['public/', 'Source/']: Use tuple not list ("(,)" not "[,]"). (Semantically equivalent, tuple is more efficient and should be preferred for any read-only list.) https://codereview.chromium.org/1213613011/diff/1/tools/git/move_source_file.... tools/git/move_source_file.py:94: return path[len(prefix):] nit: Blank line after return. https://codereview.chromium.org/1213613011/diff/1/tools/git/move_source_file.... tools/git/move_source_file.py:210: in_blink = os.getcwd().endswith("third_party/WebKit") I'm concerned this will break when the repos merge. (But maybe it's unavoidable that something like this will break.) https://codereview.chromium.org/1213613011/diff/1/tools/sort-headers.py File tools/sort-headers.py (right): https://codereview.chromium.org/1213613011/diff/1/tools/sort-headers.py#newco... tools/sort-headers.py:26: def IncludeCompareKey(line): Hmm, not really a fan of nesting this whole function just to capture for_blink. Move it back out. Add a for_blink argument to IncludeCompareKey. Then do (at the top level inside this function): compare_key = functools.partial(IncludeCompareKey, for_blink=for_blink) (see: https://docs.python.org/2/library/functools.html#functools.partial) Alternatively, if that's too much voodoo, do (as a nested function): def CompareKey(line): return IncludeCompareKey(line, for_blink) which is equivalent to functools.partial. then sorted(headerblock, key=compare_key) https://codereview.chromium.org/1213613011/diff/1/tools/sort-headers.py#newco... tools/sort-headers.py:50: return '0' Won't this conflict with <windows.h>? https://codereview.chromium.org/1213613011/diff/1/tools/sort-headers.py#newco... tools/sort-headers.py:90: perform_safety_checks, for_blink = False): No spaces around =. "for_blink=False"
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/1213613011/diff/1/tools/git/move_source_file.py File tools/git/move_source_file.py (right): https://codereview.chromium.org/1213613011/diff/1/tools/git/move_source_file.... tools/git/move_source_file.py:91: def UpdateIncludePathForBlink(path): On 2015/07/07 05:20:52, Matt Giuca wrote: > This should be moved to the top level. (Only make a nested function if you need > to close over local variables.) > > Maybe add a docstring as well to explain why. Done. https://codereview.chromium.org/1213613011/diff/1/tools/git/move_source_file.... tools/git/move_source_file.py:92: for prefix in ['public/', 'Source/']: On 2015/07/07 05:20:52, Matt Giuca wrote: > Use tuple not list ("(,)" not "[,]"). > > (Semantically equivalent, tuple is more efficient and should be preferred for > any read-only list.) Done. https://codereview.chromium.org/1213613011/diff/1/tools/git/move_source_file.... tools/git/move_source_file.py:94: return path[len(prefix):] On 2015/07/07 05:20:52, Matt Giuca wrote: > nit: Blank line after return. Done. https://codereview.chromium.org/1213613011/diff/1/tools/git/move_source_file.... tools/git/move_source_file.py:210: in_blink = os.getcwd().endswith("third_party/WebKit") On 2015/07/07 05:20:52, Matt Giuca wrote: > I'm concerned this will break when the repos merge. (But maybe it's unavoidable > that something like this will break.) Yep, it will break. I think it is unavoidable now, but will be easy to fix. https://codereview.chromium.org/1213613011/diff/1/tools/sort-headers.py File tools/sort-headers.py (right): https://codereview.chromium.org/1213613011/diff/1/tools/sort-headers.py#newco... tools/sort-headers.py:26: def IncludeCompareKey(line): On 2015/07/07 05:20:53, Matt Giuca wrote: > Hmm, not really a fan of nesting this whole function just to capture for_blink. > > Move it back out. Add a for_blink argument to IncludeCompareKey. Then do (at the > top level inside this function): > > compare_key = functools.partial(IncludeCompareKey, for_blink=for_blink) > > (see: https://docs.python.org/2/library/functools.html#functools.partial) > > Alternatively, if that's too much voodoo, do (as a nested function): > > def CompareKey(line): > return IncludeCompareKey(line, for_blink) > > which is equivalent to functools.partial. > > then > > sorted(headerblock, key=compare_key) Done the latter way as it seems more obvious. https://codereview.chromium.org/1213613011/diff/1/tools/sort-headers.py#newco... tools/sort-headers.py:50: return '0' On 2015/07/07 05:20:53, Matt Giuca wrote: > Won't this conflict with <windows.h>? Yes. From looking at blink code they don't seem to worry about doing anything special with windows.h, so I've just excluded all that special casing for blink. https://codereview.chromium.org/1213613011/diff/1/tools/sort-headers.py#newco... tools/sort-headers.py:90: perform_safety_checks, for_blink = False): On 2015/07/07 05:20:52, Matt Giuca wrote: > No spaces around =. > > "for_blink=False" Done.
https://codereview.chromium.org/1213613011/diff/40001/tools/git/move_source_f... File tools/git/move_source_file.py (right): https://codereview.chromium.org/1213613011/diff/40001/tools/git/move_source_f... tools/git/move_source_file.py:69: """Updates the path of a moved file to what it would be when used in an Docstrings should have a one-line paragraph at the top (like git commits). """<one line summary> <more stuff> """ https://codereview.chromium.org/1213613011/diff/40001/tools/git/move_source_f... tools/git/move_source_file.py:70: include statement in blink. As blink has its 'public' and 'Source' folders Nit: Capital 'B'. https://codereview.chromium.org/1213613011/diff/40001/tools/sort-headers.py File tools/sort-headers.py (right): https://codereview.chromium.org/1213613011/diff/40001/tools/sort-headers.py#n... tools/sort-headers.py:69: """Sorts the headers in infile, writing the sorted file to outfile.""" Docstring should go before the nested functions (which are technically statements of this function).
https://codereview.chromium.org/1213613011/diff/40001/tools/git/move_source_f... File tools/git/move_source_file.py (right): https://codereview.chromium.org/1213613011/diff/40001/tools/git/move_source_f... tools/git/move_source_file.py:69: """Updates the path of a moved file to what it would be when used in an On 2015/07/13 06:25:46, Matt Giuca wrote: > Docstrings should have a one-line paragraph at the top (like git commits). > > """<one line summary> > > <more stuff> > """ Done. https://codereview.chromium.org/1213613011/diff/40001/tools/git/move_source_f... tools/git/move_source_file.py:70: include statement in blink. As blink has its 'public' and 'Source' folders On 2015/07/13 06:25:46, Matt Giuca wrote: > Nit: Capital 'B'. Done. https://codereview.chromium.org/1213613011/diff/40001/tools/sort-headers.py File tools/sort-headers.py (right): https://codereview.chromium.org/1213613011/diff/40001/tools/sort-headers.py#n... tools/sort-headers.py:69: """Sorts the headers in infile, writing the sorted file to outfile.""" On 2015/07/13 06:25:46, Matt Giuca wrote: > Docstring should go before the nested functions (which are technically > statements of this function). Done.
lgtm
benwells@chromium.org changed reviewers: + satorux@chromium.org
satorux: I think you have made a lot of changes to these scripts. Wanna review? This will have to change after the repos merge, but shouldn't be too hard to do...
sorry for the belated response. https://codereview.chromium.org/1213613011/diff/60001/tools/git/move_source_f... File tools/git/move_source_file.py (right): https://codereview.chromium.org/1213613011/diff/60001/tools/git/move_source_f... tools/git/move_source_file.py:72: these prefixes of file paths are not included in include statements. might be easier to understand with examples: 'public/foo/bar.h' -> 'foo/bar.h' 'Source/foo/bar.h' -> 'foo/bar.h' https://codereview.chromium.org/1213613011/diff/60001/tools/sort-headers.py File tools/sort-headers.py (right): https://codereview.chromium.org/1213613011/diff/60001/tools/sort-headers.py#n... tools/sort-headers.py:27: Returns the filename without the #include/#import/import prefix. The description on the return value looks not correct. could you update ?
https://codereview.chromium.org/1213613011/diff/60001/tools/git/move_source_f... File tools/git/move_source_file.py (right): https://codereview.chromium.org/1213613011/diff/60001/tools/git/move_source_f... tools/git/move_source_file.py:72: these prefixes of file paths are not included in include statements. On 2015/07/14 03:41:09, satorux1 wrote: > might be easier to understand with examples: > > 'public/foo/bar.h' -> 'foo/bar.h' > 'Source/foo/bar.h' -> 'foo/bar.h' Good idea. How is this? https://codereview.chromium.org/1213613011/diff/60001/tools/sort-headers.py File tools/sort-headers.py (right): https://codereview.chromium.org/1213613011/diff/60001/tools/sort-headers.py#n... tools/sort-headers.py:27: Returns the filename without the #include/#import/import prefix. On 2015/07/14 03:41:09, satorux1 wrote: > The description on the return value looks not correct. could you update ? Done.
LGTM
The CQ bit was checked by benwells@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mgiuca@chromium.org Link to the patchset: https://codereview.chromium.org/1213613011/#ps80001 (title: "Feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1213613011/80001
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/5c5d6f9969910908c0bb4e8521077ee8c9d27c16 Cr-Commit-Position: refs/heads/master@{#338824} |
