[webkit-reviews] review denied: [Bug 72243] Add a tool to analyze change logs : [Attachment 117290] Fixed per Eric's comment
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Nov 30 16:34:49 PST 2011
Eric Seidel <eric at webkit.org> has denied Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 72243: Add a tool to analyze change logs
https://bugs.webkit.org/show_bug.cgi?id=72243
Attachment 117290: Fixed per Eric's comment
https://bugs.webkit.org/attachment.cgi?id=117290&action=review
------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=117290&action=review
I didn't review the html/js at all. The python looks generally right, but
probably needs one more round.
> Tools/Scripts/webkitpy/common/checkout/changelog.py:279
> + if rolled_over_regexp.match(line):
> + break
You should comment about why break is OK. I'm sorry I didn't make that clear
in my previous commetn. Nothing big, but it's just not immediately obvious why
we'd terminate the loop here.
> Tools/Scripts/webkitpy/common/checkout/changelog.py:288
> + yield ChangeLogEntry(''.join(entry_lines[:-1]))
> +
Why do we add this one at the end? Is this part of the chagne tested? This
could make a good separate patch w/ test case. :)
> Tools/Scripts/webkitpy/common/config/contributionareas.py:181
> + def names(self):
> + return [area.name() for area in self._contribution_areas]
I think map(operator.namegetter('name'), self._contribution_areas) is another
way to write this. I'm not sure if that's any more clear or concise though.
> Tools/Scripts/webkitpy/tool/commands/analyzechangelog.py:32
> +import time
> +from webkitpy.common.checkout.scm.detection import SCMDetector
I normally put a blank line between system imports and webkitpy imports, but I
don't think that's any sort of official style. You don't need to chagne it,
just letting you know.
> Tools/Scripts/webkitpy/tool/commands/analyzechangelog.py:48
> + steps.Options.changelog_count,
What does this do? Is this how many changelogs back to search? Feels a bit
strangely named. Feels like it's more interesting to think about searching
back to a specific date instead of a number of Changelogs. the options "git
log" has make sense here. :)
> Tools/Scripts/webkitpy/tool/commands/analyzechangelog.py:57
> + def _enumerate_changelogs(filesystem, dirname, changelog_count):
> + changelogs = [filesystem.join(dirname, filename) for filename in
filesystem.listdir(dirname) if re.match('^ChangeLog(-(\d{4}-\d{2}-\d{2}))?$',
filename)]
> + # Make sure ChangeLog shows up before ChangeLog-2011-01-01
> + changelogs = sorted(changelogs, key=lambda filename: filename + 'X',
reverse=True)
> + return changelogs[:changelog_count]
Yeah, again I think we want to end up just sorting the entries instead of the
logs themselves. :) That doesn't have to be this change, but i'm interested in
your feel for that eventual design, if it makes sense.
> Tools/Scripts/webkitpy/tool/commands/analyzechangelog.py:60
> + def _generate_jsons(filesystem, json_files, output_dir):
jsons? json_files?
> Tools/Scripts/webkitpy/tool/commands/analyzechangelog.py:67
> + output_file =
filesystem.open_text_file_for_writing(filesystem.join(output_dir, filename))
> + try:
> + print ' Generating', filename
> + output_file.write(json.dumps(json_files[filename],
indent=2))
> + finally:
> + output_file.close()
I think you want to use "with" instead. That will autoclose at the end of the
with block. Also, why not just use write_text_file directly? Or even
json.dump? (which can take a filehandle)! So many better ways...
> Tools/Scripts/webkitpy/tool/commands/analyzechangelog.py:96
> + commands_dir = filesystem.dirname(filesystem.abspath(__file__))
You want filesystem.path_to_module(self.__module__) instead, I think.
> Tools/Scripts/webkitpy/tool/commands/analyzechangelog.py:108
> + self._filesystem = host.filesystem
> + self._contribution_areas = ContributionAreas(host.filesystem)
> + self._scm = host.scm()
I'm curious why you store the filesystem and scm separately instead of just
storing the host?
> Tools/Scripts/webkitpy/tool/commands/analyzechangelog.py:128
> + status = new_status + (' ' * max(len(self._previous_status) -
len(new_status) - 1, 0))
local var for the max result might be helpful to clarity here.
> Tools/Scripts/webkitpy/tool/commands/analyzechangelog.py:129
> + print "\b" * len(self._previous_status) + status,
\b?
> Tools/Scripts/webkitpy/tool/commands/analyzechangelog.py:146
> + changelog = self._filesystem.open_text_file_for_reading(path)
> + try:
> + self._print_status('Parsing entries...')
> + number_of_parsed_entries =
self._analyze_entries(ChangeLog.parse_entries_from_file(changelog), path)
> + finally:
> + changelog.close()
Yup, again, "with" or "write_text_file" are better options.
> Tools/Scripts/webkitpy/tool/commands/analyzechangelog.py:156
> + area_contributors[contributor] = {'reviews': 0, 'reviewed': 0,
'unreviewed': 0}
These dictionaries start to feel like objects (with default values in their
constructor).
> Tools/Scripts/webkitpy/tool/commands/analyzechangelog.py:173
> + if contribution_type == 'reviews':
> + statistics['total'] += 1
> + elif reviewed:
> + statistics['reviewed'] += 1
> + else:
> + statistics['unreviewed'] += 1
Yeah, you type these strings for these dictionaries several times here. :)
> Tools/Scripts/webkitpy/tool/commands/analyzechangelog.py:182
> + def _increment_dictionary_value(self, dictionary, key):
> + dictionary[key] = dictionary.get(key, 0) + 1
Does setdefault() get you this?
> Tools/Scripts/webkitpy/tool/commands/analyzechangelog.py:204
> + for reviewer in reviewers_for_entry:
> + self._collect_statistics_for_contributor(reviewer.full_name,
'reviews', areas_for_entry, touchedfiles_for_entry, reviewed=True)
> +
> + for author in authors_for_entry:
> + self._collect_statistics_for_contributor(author['name'],
'patches', areas_for_entry, touchedfiles_for_entry,
> + reviewed=bool(reviewers_for_entry))
> +
> + for area in areas_for_entry:
> + self._areas_statistics[area]['reviewed' if
reviewers_for_entry else 'unreviewed'] += 1
Should you just be passing entry to these function calls?
More information about the webkit-reviews
mailing list