Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2379993006:
Created a tool to remove duplicate includes between h and cc files. (Closed)
DescriptionCreated a tool to remove duplicate includes between h and cc files.
BUG=
Committed: https://crrev.com/1ce6ac5081ca02d589b419dad66470d68377bc41
Cr-Commit-Position: refs/heads/master@{#423018}
Patch Set 1 #Patch Set 2 : Making file executable and reworking dry-run. #
Total comments: 12
Patch Set 3 : Updates for Max's comments. #
Total comments: 6
Patch Set 4 : More updates for Max. #
Total comments: 8
Patch Set 5 : Updates for dpranke #Messages
Total messages: 24 (13 generated)
skym@chromium.org changed reviewers: + dpranke@chromium.org
PTAL, this is just a cleaned up version of the script used to create https://codereview.chromium.org/2387553002/
The CQ bit was checked by skym@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
maxbogue@chromium.org changed reviewers: + maxbogue@chromium.org
https://codereview.chromium.org/2379993006/diff/20001/tools/remove_duplicate_... File tools/remove_duplicate_includes.py (right): https://codereview.chromium.org/2379993006/diff/20001/tools/remove_duplicate_... tools/remove_duplicate_includes.py:26: # This could be generlized if desired, and moved to command line arguments. generalized https://codereview.chromium.org/2379993006/diff/20001/tools/remove_duplicate_... tools/remove_duplicate_includes.py:40: # The key here depends on the span-dirs flag, if specified then it will only be "A map of header files to the includes they contain. The key is the full path of the header file unless the span-dirs flag is set, in which case will only be the file name (to allow mapping between files not in the same folder)." - The purpose of the variable first - The default key case (no flag) second - The abnormal key case (flag) third https://codereview.chromium.org/2379993006/diff/20001/tools/remove_duplicate_... tools/remove_duplicate_includes.py:45: # Key is always the full path to the cc file. Explain the purpose of this variable. I think it collects all the cc files during the traversal so you can go through them after the headers have been processed? https://codereview.chromium.org/2379993006/diff/20001/tools/remove_duplicate_... tools/remove_duplicate_includes.py:50: for (dir_path, dir_name_list, file_name_list) in os.walk(absolute_root): I haven't worked with python in a while but I'm pretty sure you don't need the parens around the tuple. https://codereview.chromium.org/2379993006/diff/20001/tools/remove_duplicate_... tools/remove_duplicate_includes.py:68: h_file_key = os.path.splitext(cc_file_key)[0] + H_FILE_SUFFIX You could strip "_unittest" from the end here if it exists and suddenly this would catch duplicate includes in unittest files as well right? https://codereview.chromium.org/2379993006/diff/20001/tools/remove_duplicate_... tools/remove_duplicate_includes.py:75: data = file_handle.readlines() I'd probably just call this "lines" tbh
Okay, disregard this review until I rework the cc -> h mapping so it can handle unittests elegantly. https://codereview.chromium.org/2379993006/diff/20001/tools/remove_duplicate_... File tools/remove_duplicate_includes.py (right): https://codereview.chromium.org/2379993006/diff/20001/tools/remove_duplicate_... tools/remove_duplicate_includes.py:26: # This could be generlized if desired, and moved to command line arguments. On 2016/09/30 17:40:31, maxbogue wrote: > generalized Done. https://codereview.chromium.org/2379993006/diff/20001/tools/remove_duplicate_... tools/remove_duplicate_includes.py:40: # The key here depends on the span-dirs flag, if specified then it will only be On 2016/09/30 17:40:31, maxbogue wrote: > "A map of header files to the includes they contain. The key is the full path of > the header file unless the span-dirs flag is set, in which case will only be the > file name (to allow mapping between files not in the same folder)." > > - The purpose of the variable first > - The default key case (no flag) second > - The abnormal key case (flag) third Done. https://codereview.chromium.org/2379993006/diff/20001/tools/remove_duplicate_... tools/remove_duplicate_includes.py:45: # Key is always the full path to the cc file. On 2016/09/30 17:40:31, maxbogue wrote: > Explain the purpose of this variable. I think it collects all the cc files > during the traversal so you can go through them after the headers have been > processed? Done. https://codereview.chromium.org/2379993006/diff/20001/tools/remove_duplicate_... tools/remove_duplicate_includes.py:50: for (dir_path, dir_name_list, file_name_list) in os.walk(absolute_root): On 2016/09/30 17:40:31, maxbogue wrote: > I haven't worked with python in a while but I'm pretty sure you don't need the > parens around the tuple. Done. https://codereview.chromium.org/2379993006/diff/20001/tools/remove_duplicate_... tools/remove_duplicate_includes.py:68: h_file_key = os.path.splitext(cc_file_key)[0] + H_FILE_SUFFIX On 2016/09/30 17:40:31, maxbogue wrote: > You could strip "_unittest" from the end here if it exists and suddenly this > would catch duplicate includes in unittest files as well right? Oooooh, actually maybe I should be looking at the very first include in the .cc file and use that to calculate its appropriate .h file instead. And if it doesn't conform to the name of file.cc/file.h/file_unittest.cc then throw out a warning. https://codereview.chromium.org/2379993006/diff/20001/tools/remove_duplicate_... tools/remove_duplicate_includes.py:75: data = file_handle.readlines() On 2016/09/30 17:40:31, maxbogue wrote: > I'd probably just call this "lines" tbh How about line_list?
Updated to use the first included file as the header to de-dupe with. PTAL
The CQ bit was checked by skym@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, though my python is probably rusty. https://codereview.chromium.org/2379993006/diff/40001/tools/remove_duplicate_... File tools/remove_duplicate_includes.py (right): https://codereview.chromium.org/2379993006/diff/40001/tools/remove_duplicate_... tools/remove_duplicate_includes.py:21: parser = argparse.ArgumentParser() This should be down inside your main() function. https://codereview.chromium.org/2379993006/diff/40001/tools/remove_duplicate_... tools/remove_duplicate_includes.py:82: All of the "doing stuff" part (instead of just defining functions) should be inside an if block like this, I believe: if __name__ == "__main__": <stuff> This lets people import from the file without executing stuff. Even better is to define a main() function and then call that inside such an if, like https://cs.chromium.org/chromium/src/tools/sort-headers.py does. https://codereview.chromium.org/2379993006/diff/40001/tools/remove_duplicate_... tools/remove_duplicate_includes.py:108: with open(cc_file_path, 'r' if args.dry_run else 'r+') as file_handle Shouldn't there be a : at the end of this line? How does this work without that...?
https://codereview.chromium.org/2379993006/diff/40001/tools/remove_duplicate_... File tools/remove_duplicate_includes.py (right): https://codereview.chromium.org/2379993006/diff/40001/tools/remove_duplicate_... tools/remove_duplicate_includes.py:21: parser = argparse.ArgumentParser() On 2016/10/04 15:13:21, maxbogue wrote: > This should be down inside your main() function. Done. https://codereview.chromium.org/2379993006/diff/40001/tools/remove_duplicate_... tools/remove_duplicate_includes.py:82: On 2016/10/04 15:13:21, maxbogue wrote: > All of the "doing stuff" part (instead of just defining functions) should be > inside an if block like this, I believe: > > if __name__ == "__main__": > <stuff> > > This lets people import from the file without executing stuff. Even better is to > define a main() function and then call that inside such an if, like > https://cs.chromium.org/chromium/src/tools/sort-headers.py does. Done. https://codereview.chromium.org/2379993006/diff/40001/tools/remove_duplicate_... tools/remove_duplicate_includes.py:108: with open(cc_file_path, 'r' if args.dry_run else 'r+') as file_handle On 2016/10/04 15:13:21, maxbogue wrote: > Shouldn't there be a : at the end of this line? How does this work without > that...? You're totally right, it doesn't work! Last minute refactoring that didn't get tested. Done.
lgtm w/ a couple of stylistic comments. https://codereview.chromium.org/2379993006/diff/60001/tools/remove_duplicate_... File tools/remove_duplicate_includes.py (right): https://codereview.chromium.org/2379993006/diff/60001/tools/remove_duplicate_... tools/remove_duplicate_includes.py:6: '''This script will search through the target folder specified and try to find Use triple-double quotes (""") for docstrings, even though we prefer single quotes to double quotes for other strings (see PEP-257). https://codereview.chromium.org/2379993006/diff/60001/tools/remove_duplicate_... tools/remove_duplicate_includes.py:11: Usage remove_duplicate_includes.py --dry-run components/foo components/bar Nit: s/Usage/Usage:/. https://codereview.chromium.org/2379993006/diff/60001/tools/remove_duplicate_... tools/remove_duplicate_includes.py:37: Find and returns the corresponding include set for the given .cc file. This is s/Find/Finds/. Also, add a carriage return and a blank line after the first sentence, to make it into two paragraphs (where possible, docstrings should have a single sentence as the first line/paragraph for easier summarization). https://codereview.chromium.org/2379993006/diff/60001/tools/remove_duplicate_... tools/remove_duplicate_includes.py:109: with open(cc_file_path, 'r' if args.dry_run else 'r+') as file_handle: Nit: I'd just use 'fh' or 'fp' for the handle here; both are common enough names that they are easily recognized and that makes the code shorter as a result.
Updates https://codereview.chromium.org/2379993006/diff/60001/tools/remove_duplicate_... File tools/remove_duplicate_includes.py (right): https://codereview.chromium.org/2379993006/diff/60001/tools/remove_duplicate_... tools/remove_duplicate_includes.py:6: '''This script will search through the target folder specified and try to find On 2016/10/04 20:37:04, Dirk Pranke (slow) wrote: > Use triple-double quotes (""") for docstrings, even though we prefer single > quotes to double quotes for other strings (see PEP-257). Done. https://codereview.chromium.org/2379993006/diff/60001/tools/remove_duplicate_... tools/remove_duplicate_includes.py:11: Usage remove_duplicate_includes.py --dry-run components/foo components/bar On 2016/10/04 20:37:04, Dirk Pranke (slow) wrote: > Nit: s/Usage/Usage:/. Done. https://codereview.chromium.org/2379993006/diff/60001/tools/remove_duplicate_... tools/remove_duplicate_includes.py:37: Find and returns the corresponding include set for the given .cc file. This is On 2016/10/04 20:37:04, Dirk Pranke (slow) wrote: > s/Find/Finds/. Also, add a carriage return and a blank line after the first > sentence, to make it into two paragraphs (where possible, docstrings should have > a single sentence as the first line/paragraph for easier summarization). Done. https://codereview.chromium.org/2379993006/diff/60001/tools/remove_duplicate_... tools/remove_duplicate_includes.py:109: with open(cc_file_path, 'r' if args.dry_run else 'r+') as file_handle: On 2016/10/04 20:37:04, Dirk Pranke (slow) wrote: > Nit: I'd just use 'fh' or 'fp' for the handle here; both are common enough names > that they are easily recognized and that makes the code shorter as a result. Done.
The CQ bit was checked by skym@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from maxbogue@chromium.org, dpranke@chromium.org Link to the patchset: https://codereview.chromium.org/2379993006/#ps80001 (title: "Updates for dpranke")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Created a tool to remove duplicate includes between h and cc files. BUG= ========== to ========== Created a tool to remove duplicate includes between h and cc files. BUG= Committed: https://crrev.com/1ce6ac5081ca02d589b419dad66470d68377bc41 Cr-Commit-Position: refs/heads/master@{#423018} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/1ce6ac5081ca02d589b419dad66470d68377bc41 Cr-Commit-Position: refs/heads/master@{#423018} |
