[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