Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2648883002:
Add coverage statistic (Closed)
Descriptionadd coverage statistic
BUG=681094
R=robertocn
Review-Url: https://codereview.chromium.org/2648883002
Cr-Commit-Position: refs/heads/master@{#448739}
Committed: https://chromium.googlesource.com/chromium/src/+/7280dbb030e5a867689c2c152748d8885e085c70
Patch Set 1 #
Total comments: 13
Patch Set 2 : Add level parameter #
Total comments: 8
Patch Set 3 : add display complete stat #
Total comments: 16
Patch Set 4 : rename variables #
Total comments: 2
Patch Set 5 : update Docstring #
Messages
Total messages: 37 (17 generated)
Description was changed from ========== add coverage statistic BUG=681094 R=robertocn ========== to ========== add coverage statistic BUG=681094 R=robertocn ==========
Add team/component coverage statistic
ymzhang@chromium.org changed reviewers: + sshruthi@chromium.org
On 2017/01/20 23:46:12, ymzhang1 wrote: > Add team/component coverage statistic Can we add information about coverage in levels. For instance, for a specific directory, I would like to see: xx% of first-level directories have coverage xx% of second-level directories have coverage and so on. That will help us target directories that need more attention.
https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... File tools/checkteamtags/owners_file_tags.py (right): https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... tools/checkteamtags/owners_file_tags.py:42: A pair (data, warnings) where data is a dict of the form Change the docstring for the return value. https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... tools/checkteamtags/owners_file_tags.py:47: statis = {} I think I like 'stats' better than 'statis' as an abbreviation for statistics. https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... tools/checkteamtags/owners_file_tags.py:48: num_total = 0 Maybe make these dictionaries that associate the depth of the dir with a count. (perhaps a defaultdict) https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... tools/checkteamtags/owners_file_tags.py:49: num_compo = 0 let's name these something like 'with_component' and 'with_team_and_component' to make it more readable. Also, do we need all 4 counts? isn't num_total = num_compo + num_none https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... tools/checkteamtags/owners_file_tags.py:55: for dirname, _, files in os.walk(root): To find the depth of the directory you can find the path of 'dirname' relative to 'root' and finding the number of dirs it contains. e.g. 'len(os.path.relpath(dirname,root).split(os.sep))' or something like that. https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... tools/checkteamtags/owners_file_tags.py:76: 'have-component-OWNERS': num_compo, Should we return these as percentages as well as raw counts?
https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... File tools/checkteamtags/owners_file_tags.py (right): https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... tools/checkteamtags/owners_file_tags.py:42: A pair (data, warnings) where data is a dict of the form Change the docstring for the return value. https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... tools/checkteamtags/owners_file_tags.py:47: statis = {} I think I like 'stats' better than 'statis' as an abbreviation for statistics. https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... tools/checkteamtags/owners_file_tags.py:48: num_total = 0 Maybe make these dictionaries that associate the depth of the dir with a count. (perhaps a defaultdict) https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... tools/checkteamtags/owners_file_tags.py:49: num_compo = 0 let's name these something like 'with_component' and 'with_team_and_component' to make it more readable. Also, do we need all 4 counts? isn't num_total = num_compo + num_none https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... tools/checkteamtags/owners_file_tags.py:55: for dirname, _, files in os.walk(root): To find the depth of the directory you can find the path of 'dirname' relative to 'root' and finding the number of dirs it contains. e.g. 'len(os.path.relpath(dirname,root).split(os.sep))' or something like that. https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... tools/checkteamtags/owners_file_tags.py:76: 'have-component-OWNERS': num_compo, Should we return these as percentages as well as raw counts?
On 2017/01/25 20:40:50, RobertoCN wrote: > https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... > File tools/checkteamtags/owners_file_tags.py (right): > > https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... > tools/checkteamtags/owners_file_tags.py:42: A pair (data, warnings) where data > is a dict of the form > Change the docstring for the return value. > > https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... > tools/checkteamtags/owners_file_tags.py:47: statis = {} > I think I like 'stats' better than 'statis' as an abbreviation for statistics. > > https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... > tools/checkteamtags/owners_file_tags.py:48: num_total = 0 > Maybe make these dictionaries that associate the depth of the dir with a count. > (perhaps a defaultdict) > > https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... > tools/checkteamtags/owners_file_tags.py:49: num_compo = 0 > let's name these something like 'with_component' and 'with_team_and_component' > to make it more readable. > > Also, do we need all 4 counts? isn't num_total = num_compo + num_none > > https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... > tools/checkteamtags/owners_file_tags.py:55: for dirname, _, files in > os.walk(root): > To find the depth of the directory you can find the path of 'dirname' relative > to 'root' and finding the number of dirs it contains. e.g. > 'len(os.path.relpath(dirname,root).split(os.sep))' or something like that. > > https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... > tools/checkteamtags/owners_file_tags.py:76: 'have-component-OWNERS': num_compo, > Should we return these as percentages as well as raw counts? Yes, please!
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
Have added level by level statistic besides total statistic. Currently I print the statistic for all depths. Here's an example under third_party/WebKit. Feel free to let me know if you would like to change format or add/delete something. 148 OWNERS files in total. 60 (40.54%) OWNERS files have COMPONENT 48 (32.43%) OWNERS files have TEAM&COMPONENT Under directory ../../third_party/WebKit/ 6 OWNERS files at depth 0 have COMPONENT: 2, percentage: 33.33% have COMPONENT and TEAM: 1,percentage: 16.67% 12 OWNERS files at depth 1 have COMPONENT: 6, percentage: 50.00% have COMPONENT and TEAM: 5,percentage: 41.67% 92 OWNERS files at depth 2 have COMPONENT: 41, percentage: 44.57% have COMPONENT and TEAM: 33,percentage: 35.87% 34 OWNERS files at depth 3 have COMPONENT: 11, percentage: 32.35% have COMPONENT and TEAM: 9,percentage: 26.47% 1 OWNERS files at depth 4 have COMPONENT: 0, percentage: 0.00% have COMPONENT and TEAM: 0,percentage: 0.00% 2 OWNERS files at depth 5 have COMPONENT: 0, percentage: 0.00% have COMPONENT and TEAM: 0,percentage: 0.00% 1 OWNERS files at depth 6 have COMPONENT: 0, percentage: 0.00% have COMPONENT and TEAM: 0,percentage: 0.00% https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... File tools/checkteamtags/owners_file_tags.py (right): https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... tools/checkteamtags/owners_file_tags.py:42: A pair (data, warnings) where data is a dict of the form On 2017/01/25 20:40:50, RobertoCN wrote: > Change the docstring for the return value. Done. https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... tools/checkteamtags/owners_file_tags.py:42: A pair (data, warnings) where data is a dict of the form On 2017/01/25 20:40:50, RobertoCN wrote: > Change the docstring for the return value. Done. https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... tools/checkteamtags/owners_file_tags.py:47: statis = {} On 2017/01/25 20:40:50, RobertoCN wrote: > I think I like 'stats' better than 'statis' as an abbreviation for statistics. Done. https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... tools/checkteamtags/owners_file_tags.py:48: num_total = 0 On 2017/01/25 20:40:50, RobertoCN wrote: > Maybe make these dictionaries that associate the depth of the dir with a count. > (perhaps a defaultdict) Done. https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... tools/checkteamtags/owners_file_tags.py:49: num_compo = 0 On 2017/01/25 20:40:50, RobertoCN wrote: > let's name these something like 'with_component' and 'with_team_and_component' > to make it more readable. > > Also, do we need all 4 counts? isn't num_total = num_compo + num_none Done. https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... tools/checkteamtags/owners_file_tags.py:55: for dirname, _, files in os.walk(root): On 2017/01/25 20:40:50, RobertoCN wrote: > To find the depth of the directory you can find the path of 'dirname' relative > to 'root' and finding the number of dirs it contains. e.g. > 'len(os.path.relpath(dirname,root).split(os.sep))' or something like that. Thanks for the suggestion! https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... tools/checkteamtags/owners_file_tags.py:76: 'have-component-OWNERS': num_compo, On 2017/01/25 20:40:50, RobertoCN wrote: > Should we return these as percentages as well as raw counts? Sure. I'll computer the percentage when output on screen.
On 2017/01/27 23:24:51, ymzhang1 wrote: > Have added level by level statistic besides total statistic. Currently I print > the statistic for all depths. > > Here's an example under third_party/WebKit. Feel free to let me know if you > would like to change format or add/delete something. > > 148 OWNERS files in total. > 60 (40.54%) OWNERS files have COMPONENT > 48 (32.43%) OWNERS files have TEAM&COMPONENT > > Under directory ../../third_party/WebKit/ > 6 OWNERS files at depth 0 > have COMPONENT: 2, percentage: 33.33% > have COMPONENT and TEAM: 1,percentage: 16.67% > 12 OWNERS files at depth 1 > have COMPONENT: 6, percentage: 50.00% > have COMPONENT and TEAM: 5,percentage: 41.67% > 92 OWNERS files at depth 2 > have COMPONENT: 41, percentage: 44.57% > have COMPONENT and TEAM: 33,percentage: 35.87% > 34 OWNERS files at depth 3 > have COMPONENT: 11, percentage: 32.35% > have COMPONENT and TEAM: 9,percentage: 26.47% > 1 OWNERS files at depth 4 > have COMPONENT: 0, percentage: 0.00% > have COMPONENT and TEAM: 0,percentage: 0.00% > 2 OWNERS files at depth 5 > have COMPONENT: 0, percentage: 0.00% > have COMPONENT and TEAM: 0,percentage: 0.00% > 1 OWNERS files at depth 6 > have COMPONENT: 0, percentage: 0.00% > have COMPONENT and TEAM: 0,percentage: 0.00% > > https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... > File tools/checkteamtags/owners_file_tags.py (right): > > https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... > tools/checkteamtags/owners_file_tags.py:42: A pair (data, warnings) where data > is a dict of the form > On 2017/01/25 20:40:50, RobertoCN wrote: > > Change the docstring for the return value. > > Done. > > https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... > tools/checkteamtags/owners_file_tags.py:42: A pair (data, warnings) where data > is a dict of the form > On 2017/01/25 20:40:50, RobertoCN wrote: > > Change the docstring for the return value. > > Done. > > https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... > tools/checkteamtags/owners_file_tags.py:47: statis = {} > On 2017/01/25 20:40:50, RobertoCN wrote: > > I think I like 'stats' better than 'statis' as an abbreviation for statistics. > > Done. > > https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... > tools/checkteamtags/owners_file_tags.py:48: num_total = 0 > On 2017/01/25 20:40:50, RobertoCN wrote: > > Maybe make these dictionaries that associate the depth of the dir with a > count. > > (perhaps a defaultdict) > > Done. > > https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... > tools/checkteamtags/owners_file_tags.py:49: num_compo = 0 > On 2017/01/25 20:40:50, RobertoCN wrote: > > let's name these something like 'with_component' and 'with_team_and_component' > > to make it more readable. > > > > Also, do we need all 4 counts? isn't num_total = num_compo + num_none > > Done. > > https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... > tools/checkteamtags/owners_file_tags.py:55: for dirname, _, files in > os.walk(root): > On 2017/01/25 20:40:50, RobertoCN wrote: > > To find the depth of the directory you can find the path of 'dirname' relative > > to 'root' and finding the number of dirs it contains. e.g. > > 'len(os.path.relpath(dirname,root).split(os.sep))' or something like that. > > Thanks for the suggestion! > > https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... > tools/checkteamtags/owners_file_tags.py:76: 'have-component-OWNERS': num_compo, > On 2017/01/25 20:40:50, RobertoCN wrote: > > Should we return these as percentages as well as raw counts? > > Sure. I'll computer the percentage when output on screen. Can we add an option to stop at a certain level? That would be useful when we want to check stats for all of chromium/src to share widly.
On 2017/01/27 23:30:20, sshruthi wrote: > On 2017/01/27 23:24:51, ymzhang1 wrote: > > Have added level by level statistic besides total statistic. Currently I print > > the statistic for all depths. > > > > Here's an example under third_party/WebKit. Feel free to let me know if you > > would like to change format or add/delete something. > > > > 148 OWNERS files in total. > > 60 (40.54%) OWNERS files have COMPONENT > > 48 (32.43%) OWNERS files have TEAM&COMPONENT > > > > Under directory ../../third_party/WebKit/ > > 6 OWNERS files at depth 0 > > have COMPONENT: 2, percentage: 33.33% > > have COMPONENT and TEAM: 1,percentage: 16.67% > > 12 OWNERS files at depth 1 > > have COMPONENT: 6, percentage: 50.00% > > have COMPONENT and TEAM: 5,percentage: 41.67% > > 92 OWNERS files at depth 2 > > have COMPONENT: 41, percentage: 44.57% > > have COMPONENT and TEAM: 33,percentage: 35.87% > > 34 OWNERS files at depth 3 > > have COMPONENT: 11, percentage: 32.35% > > have COMPONENT and TEAM: 9,percentage: 26.47% > > 1 OWNERS files at depth 4 > > have COMPONENT: 0, percentage: 0.00% > > have COMPONENT and TEAM: 0,percentage: 0.00% > > 2 OWNERS files at depth 5 > > have COMPONENT: 0, percentage: 0.00% > > have COMPONENT and TEAM: 0,percentage: 0.00% > > 1 OWNERS files at depth 6 > > have COMPONENT: 0, percentage: 0.00% > > have COMPONENT and TEAM: 0,percentage: 0.00% > > > > > https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... > > File tools/checkteamtags/owners_file_tags.py (right): > > > > > https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... > > tools/checkteamtags/owners_file_tags.py:42: A pair (data, warnings) where data > > is a dict of the form > > On 2017/01/25 20:40:50, RobertoCN wrote: > > > Change the docstring for the return value. > > > > Done. > > > > > https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... > > tools/checkteamtags/owners_file_tags.py:42: A pair (data, warnings) where data > > is a dict of the form > > On 2017/01/25 20:40:50, RobertoCN wrote: > > > Change the docstring for the return value. > > > > Done. > > > > > https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... > > tools/checkteamtags/owners_file_tags.py:47: statis = {} > > On 2017/01/25 20:40:50, RobertoCN wrote: > > > I think I like 'stats' better than 'statis' as an abbreviation for > statistics. > > > > Done. > > > > > https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... > > tools/checkteamtags/owners_file_tags.py:48: num_total = 0 > > On 2017/01/25 20:40:50, RobertoCN wrote: > > > Maybe make these dictionaries that associate the depth of the dir with a > > count. > > > (perhaps a defaultdict) > > > > Done. > > > > > https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... > > tools/checkteamtags/owners_file_tags.py:49: num_compo = 0 > > On 2017/01/25 20:40:50, RobertoCN wrote: > > > let's name these something like 'with_component' and > 'with_team_and_component' > > > to make it more readable. > > > > > > Also, do we need all 4 counts? isn't num_total = num_compo + num_none > > > > Done. > > > > > https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... > > tools/checkteamtags/owners_file_tags.py:55: for dirname, _, files in > > os.walk(root): > > On 2017/01/25 20:40:50, RobertoCN wrote: > > > To find the depth of the directory you can find the path of 'dirname' > relative > > > to 'root' and finding the number of dirs it contains. e.g. > > > 'len(os.path.relpath(dirname,root).split(os.sep))' or something like that. > > > > Thanks for the suggestion! > > > > > https://codereview.chromium.org/2648883002/diff/1/tools/checkteamtags/owners_... > > tools/checkteamtags/owners_file_tags.py:76: 'have-component-OWNERS': > num_compo, > > On 2017/01/25 20:40:50, RobertoCN wrote: > > > Should we return these as percentages as well as raw counts? > > > > Sure. I'll computer the percentage when output on screen. > > Can we add an option to stop at a certain level? That would be useful when we > want to check stats for all of chromium/src to share widly. Sure, I'll work on that.
Patchset #2 (id:100001) has been deleted
Patchset #2 (id:120001) has been deleted
Patchset #2 (id:140001) has been deleted
Could specify how many level to display. e.g. python extract_components.py -s 3 ../../third_party/WebKit/ Result: 148 OWNERS files in total. 65 (43.92%) OWNERS files have COMPONENT 50 (33.78%) OWNERS files have TEAM&COMPONENT Under directory ../../third_party/WebKit/ 6 OWNERS files at depth 0 have COMPONENT: 2, percentage: 33.33% have COMPONENT and TEAM: 1,percentage: 16.67% 12 OWNERS files at depth 1 have COMPONENT: 6, percentage: 50.00% have COMPONENT and TEAM: 5,percentage: 41.67% 92 OWNERS files at depth 2 have COMPONENT: 46, percentage: 50.00% have COMPONENT and TEAM: 35,percentage: 38.04%
https://codereview.chromium.org/2648883002/diff/160001/tools/checkteamtags/ex... File tools/checkteamtags/extract_components.py (right): https://codereview.chromium.org/2648883002/diff/160001/tools/checkteamtags/ex... tools/checkteamtags/extract_components.py:59: python %prog -s 3 /b/build/src What happens if we pass -s, but don't pass a number? Is there a way to request unlimited depth? https://codereview.chromium.org/2648883002/diff/160001/tools/checkteamtags/ex... tools/checkteamtags/extract_components.py:88: file_total = stats['total-number-OWNERS'] Consider moving this functionality (display stats) to its own function with a docstring explaining it. https://codereview.chromium.org/2648883002/diff/160001/tools/checkteamtags/ex... tools/checkteamtags/extract_components.py:110: num_output_depth = len(stats['file-total']) Could you comment on what num_output_depth is? and why is it initialized to len(stats['file-total']) https://codereview.chromium.org/2648883002/diff/160001/tools/checkteamtags/ex... tools/checkteamtags/extract_components.py:114: for depth_id in range(0, num_output_depth): Maybe depth_id should be called just depth or level?
On 2017/01/31 23:04:28, RobertoCN wrote: > https://codereview.chromium.org/2648883002/diff/160001/tools/checkteamtags/ex... > File tools/checkteamtags/extract_components.py (right): > > https://codereview.chromium.org/2648883002/diff/160001/tools/checkteamtags/ex... > tools/checkteamtags/extract_components.py:59: python %prog -s 3 /b/build/src > What happens if we pass -s, but don't pass a number? > > Is there a way to request unlimited depth? > > https://codereview.chromium.org/2648883002/diff/160001/tools/checkteamtags/ex... > tools/checkteamtags/extract_components.py:88: file_total = > stats['total-number-OWNERS'] > Consider moving this functionality (display stats) to its own function with a > docstring explaining it. > > https://codereview.chromium.org/2648883002/diff/160001/tools/checkteamtags/ex... > tools/checkteamtags/extract_components.py:110: num_output_depth = > len(stats['file-total']) > Could you comment on what num_output_depth is? and why is it initialized to > len(stats['file-total']) > > https://codereview.chromium.org/2648883002/diff/160001/tools/checkteamtags/ex... > tools/checkteamtags/extract_components.py:114: for depth_id in range(0, > num_output_depth): > Maybe depth_id should be called just depth or level? Yes. If the input depth is invalid. For example, -1, then unlimited depth will be returned.
On 2017/01/31 23:06:52, ymzhang1 wrote: > On 2017/01/31 23:04:28, RobertoCN wrote: > > > https://codereview.chromium.org/2648883002/diff/160001/tools/checkteamtags/ex... > > File tools/checkteamtags/extract_components.py (right): > > > > > https://codereview.chromium.org/2648883002/diff/160001/tools/checkteamtags/ex... > > tools/checkteamtags/extract_components.py:59: python %prog -s 3 /b/build/src > > What happens if we pass -s, but don't pass a number? > > > > Is there a way to request unlimited depth? > > > > > https://codereview.chromium.org/2648883002/diff/160001/tools/checkteamtags/ex... > > tools/checkteamtags/extract_components.py:88: file_total = > > stats['total-number-OWNERS'] > > Consider moving this functionality (display stats) to its own function with a > > docstring explaining it. > > > > > https://codereview.chromium.org/2648883002/diff/160001/tools/checkteamtags/ex... > > tools/checkteamtags/extract_components.py:110: num_output_depth = > > len(stats['file-total']) > > Could you comment on what num_output_depth is? and why is it initialized to > > len(stats['file-total']) > > > > > https://codereview.chromium.org/2648883002/diff/160001/tools/checkteamtags/ex... > > tools/checkteamtags/extract_components.py:114: for depth_id in range(0, > > num_output_depth): > > Maybe depth_id should be called just depth or level? > > Yes. If the input depth is invalid. For example, -1, then unlimited depth will > be returned. Can we make it so that if no depth is specified then it is treated like -1?
On 2017/01/31 23:22:56, RobertoCN wrote: > On 2017/01/31 23:06:52, ymzhang1 wrote: > > On 2017/01/31 23:04:28, RobertoCN wrote: > > > > > > https://codereview.chromium.org/2648883002/diff/160001/tools/checkteamtags/ex... > > > File tools/checkteamtags/extract_components.py (right): > > > > > > > > > https://codereview.chromium.org/2648883002/diff/160001/tools/checkteamtags/ex... > > > tools/checkteamtags/extract_components.py:59: python %prog -s 3 /b/build/src > > > What happens if we pass -s, but don't pass a number? > > > > > > Is there a way to request unlimited depth? > > > > > > > > > https://codereview.chromium.org/2648883002/diff/160001/tools/checkteamtags/ex... > > > tools/checkteamtags/extract_components.py:88: file_total = > > > stats['total-number-OWNERS'] > > > Consider moving this functionality (display stats) to its own function with > a > > > docstring explaining it. > > > > > > > > > https://codereview.chromium.org/2648883002/diff/160001/tools/checkteamtags/ex... > > > tools/checkteamtags/extract_components.py:110: num_output_depth = > > > len(stats['file-total']) > > > Could you comment on what num_output_depth is? and why is it initialized to > > > len(stats['file-total']) > > > > > > > > > https://codereview.chromium.org/2648883002/diff/160001/tools/checkteamtags/ex... > > > tools/checkteamtags/extract_components.py:114: for depth_id in range(0, > > > num_output_depth): > > > Maybe depth_id should be called just depth or level? > > > > Yes. If the input depth is invalid. For example, -1, then unlimited depth will > > be returned. > > Can we make it so that if no depth is specified then it is treated like -1? Yes, I think so. I will update and let you know.
Patchset #3 (id:180001) has been deleted
Patchset #3 (id:200001) has been deleted
https://codereview.chromium.org/2648883002/diff/160001/tools/checkteamtags/ex... File tools/checkteamtags/extract_components.py (right): https://codereview.chromium.org/2648883002/diff/160001/tools/checkteamtags/ex... tools/checkteamtags/extract_components.py:59: python %prog -s 3 /b/build/src On 2017/01/31 23:04:28, RobertoCN wrote: > What happens if we pass -s, but don't pass a number? > > Is there a way to request unlimited depth? If only pass -s, would raise error. -s require an integer input. I add -c to dispaly unlimited depth. https://codereview.chromium.org/2648883002/diff/160001/tools/checkteamtags/ex... tools/checkteamtags/extract_components.py:88: file_total = stats['total-number-OWNERS'] On 2017/01/31 23:04:28, RobertoCN wrote: > Consider moving this functionality (display stats) to its own function with a > docstring explaining it. Done. https://codereview.chromium.org/2648883002/diff/160001/tools/checkteamtags/ex... tools/checkteamtags/extract_components.py:110: num_output_depth = len(stats['file-total']) On 2017/01/31 23:04:28, RobertoCN wrote: > Could you comment on what num_output_depth is? and why is it initialized to > len(stats['file-total']) Done. https://codereview.chromium.org/2648883002/diff/160001/tools/checkteamtags/ex... tools/checkteamtags/extract_components.py:114: for depth_id in range(0, num_output_depth): On 2017/01/31 23:04:28, RobertoCN wrote: > Maybe depth_id should be called just depth or level? Done.
I have a few comments, mostly about style and naming. The logic looks good, though. https://codereview.chromium.org/2648883002/diff/220001/tools/checkteamtags/ex... File tools/checkteamtags/extract_components.py (right): https://codereview.chromium.org/2648883002/diff/220001/tools/checkteamtags/ex... tools/checkteamtags/extract_components.py:48: """"display coverage statistic Nit: spellcheck this docstring and add Args: section. https://codereview.chromium.org/2648883002/diff/220001/tools/checkteamtags/ex... tools/checkteamtags/extract_components.py:61: file_per_with_compo = "N/A" Nit: where practical avoid non-standard abbreviations in variable names: https://engdoc.corp.google.com/eng/doc/devguide/py/styleguide.shtml?cl=head#n... In this case _per_ is confusing, as it is not obvious that it stands for percent. If you really must abbreviate it, use a more standard abbreviation such as 'pct' compo is a little more understandable, but I would prefer if it said component. https://codereview.chromium.org/2648883002/diff/220001/tools/checkteamtags/ex... tools/checkteamtags/extract_components.py:75: 'OWNERS files have TEAM&COMPONENT' % { Nit: TEAM and COMPONENT https://codereview.chromium.org/2648883002/diff/220001/tools/checkteamtags/ex... tools/checkteamtags/extract_components.py:88: cnt_total = stats['file-total'][depth] Abbr. https://codereview.chromium.org/2648883002/diff/220001/tools/checkteamtags/ex... tools/checkteamtags/extract_components.py:124: python %prog /b/build/src -s 3 shouldn't we say `-s 3 /b/build/src` ? https://codereview.chromium.org/2648883002/diff/220001/tools/checkteamtags/ow... File tools/checkteamtags/owners_file_tags.py (right): https://codereview.chromium.org/2648883002/diff/220001/tools/checkteamtags/ow... tools/checkteamtags/owners_file_tags.py:42: A pair (data, warnings, errors, stats) where data is a dict of the form Nit: If it has > 2 values, it is a tuple instead of a pair https://codereview.chromium.org/2648883002/diff/220001/tools/checkteamtags/ow... tools/checkteamtags/owners_file_tags.py:46: {'total-number-OWNERS': total number of OWNERS files, I would use keys like: OWNERS-count OWNERS-with-component-only-count OWNERS-with-component-and-team-count OWNERS-count-by-depth OWNERS-with-component-only-count-by-depth OWNERS-with-component-and-team-count-by-depth (Better long and descriptive) https://codereview.chromium.org/2648883002/diff/220001/tools/checkteamtags/ow... tools/checkteamtags/owners_file_tags.py:48: 'have-team-componnet-OWNERS': number of OWNERS have TEAM, COMPONENT, spelling
Thanks for the detail comments and suggestions! https://codereview.chromium.org/2648883002/diff/220001/tools/checkteamtags/ex... File tools/checkteamtags/extract_components.py (right): https://codereview.chromium.org/2648883002/diff/220001/tools/checkteamtags/ex... tools/checkteamtags/extract_components.py:48: """"display coverage statistic On 2017/02/06 21:55:31, RobertoCN wrote: > Nit: spellcheck this docstring and add Args: section. Done. https://codereview.chromium.org/2648883002/diff/220001/tools/checkteamtags/ex... tools/checkteamtags/extract_components.py:61: file_per_with_compo = "N/A" On 2017/02/06 21:55:31, RobertoCN wrote: > Nit: where practical avoid non-standard abbreviations in variable names: > https://engdoc.corp.google.com/eng/doc/devguide/py/styleguide.shtml?cl=head#n... > > In this case _per_ is confusing, as it is not obvious that it stands for > percent. If you really must abbreviate it, use a more standard abbreviation such > as 'pct' > > compo is a little more understandable, but I would prefer if it said component. Done. https://codereview.chromium.org/2648883002/diff/220001/tools/checkteamtags/ex... tools/checkteamtags/extract_components.py:75: 'OWNERS files have TEAM&COMPONENT' % { On 2017/02/06 21:55:31, RobertoCN wrote: > Nit: TEAM and COMPONENT Done. https://codereview.chromium.org/2648883002/diff/220001/tools/checkteamtags/ex... tools/checkteamtags/extract_components.py:88: cnt_total = stats['file-total'][depth] On 2017/02/06 21:55:31, RobertoCN wrote: > Abbr. Done. https://codereview.chromium.org/2648883002/diff/220001/tools/checkteamtags/ex... tools/checkteamtags/extract_components.py:124: python %prog /b/build/src -s 3 On 2017/02/06 21:55:31, RobertoCN wrote: > shouldn't we say `-s 3 /b/build/src` ? Done. https://codereview.chromium.org/2648883002/diff/220001/tools/checkteamtags/ow... File tools/checkteamtags/owners_file_tags.py (right): https://codereview.chromium.org/2648883002/diff/220001/tools/checkteamtags/ow... tools/checkteamtags/owners_file_tags.py:42: A pair (data, warnings, errors, stats) where data is a dict of the form On 2017/02/06 21:55:31, RobertoCN wrote: > Nit: If it has > 2 values, it is a tuple instead of a pair Done. https://codereview.chromium.org/2648883002/diff/220001/tools/checkteamtags/ow... tools/checkteamtags/owners_file_tags.py:46: {'total-number-OWNERS': total number of OWNERS files, On 2017/02/06 21:55:31, RobertoCN wrote: > I would use keys like: > OWNERS-count > OWNERS-with-component-only-count > OWNERS-with-component-and-team-count > OWNERS-count-by-depth > OWNERS-with-component-only-count-by-depth > OWNERS-with-component-and-team-count-by-depth > > (Better long and descriptive) Done. https://codereview.chromium.org/2648883002/diff/220001/tools/checkteamtags/ow... tools/checkteamtags/owners_file_tags.py:48: 'have-team-componnet-OWNERS': number of OWNERS have TEAM, COMPONENT, On 2017/02/06 21:55:31, RobertoCN wrote: > spelling Done.
LGTM % Docstring. https://codereview.chromium.org/2648883002/diff/240001/tools/checkteamtags/ex... File tools/checkteamtags/extract_components.py (right): https://codereview.chromium.org/2648883002/diff/240001/tools/checkteamtags/ex... tools/checkteamtags/extract_components.py:48: """"display coverage statistic """Display coverage statistics. The following three values are always displayed: - The total number of OWNERS files under directory root and its sub- directories. - The number of OWNERS files (and its percentage of the total) that have component information but no team information. - The number of OWNERS files (and its percentage of the total) that have both component and team information. Optionally, if options.stat_coverage or options.complete_coverage are given, the same information will be shown for each depth level. (up to the level given by options.stat_coverage, if any). Args: stats (dict): The statistics in dictionary form as produced by the owners_file_tags module. root (str): The root directory from which the depth level is calculated. options (optparse.Values): The command line options as returned by optparse."""
Patchset #5 (id:260001) has been deleted
The CQ bit was checked by ymzhang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robertocn@chromium.org Link to the patchset: https://codereview.chromium.org/2648883002/#ps280001 (title: "update Docstring")
https://codereview.chromium.org/2648883002/diff/240001/tools/checkteamtags/ex... File tools/checkteamtags/extract_components.py (right): https://codereview.chromium.org/2648883002/diff/240001/tools/checkteamtags/ex... tools/checkteamtags/extract_components.py:48: """"display coverage statistic On 2017/02/07 20:19:30, RobertoCN wrote: > """Display coverage statistics. > > The following three values are always displayed: > - The total number of OWNERS files under directory root and its sub- > directories. > - The number of OWNERS files (and its percentage of the total) that > have component information but no team information. > - The number of OWNERS files (and its percentage of the total) that > have both component and team information. > > Optionally, if options.stat_coverage or options.complete_coverage are > given, the same information will be shown for each depth level. > (up to the level given by options.stat_coverage, if any). > > Args: > stats (dict): The statistics in dictionary form as produced by the > owners_file_tags module. > root (str): The root directory from which the depth level is > calculated. > options (optparse.Values): The command line options as returned by > optparse.""" Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 280001, "attempt_start_ts": 1486502119076690,
"parent_rev": "45a965f8519020883364af56fc28b8164bd5751a", "commit_rev":
"7280dbb030e5a867689c2c152748d8885e085c70"}
CQ is committing da patch.
Bot data: {"patchset_id": 280001, "attempt_start_ts": 1486502119076690,
"parent_rev": "45a965f8519020883364af56fc28b8164bd5751a", "commit_rev":
"7280dbb030e5a867689c2c152748d8885e085c70"}
Message was sent while issue was closed.
Description was changed from ========== add coverage statistic BUG=681094 R=robertocn ========== to ========== add coverage statistic BUG=681094 R=robertocn Review-Url: https://codereview.chromium.org/2648883002 Cr-Commit-Position: refs/heads/master@{#448739} Committed: https://chromium.googlesource.com/chromium/src/+/7280dbb030e5a867689c2c152748... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:280001) as https://chromium.googlesource.com/chromium/src/+/7280dbb030e5a867689c2c152748... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
