Code review comment for lp:~gagern/bzr/bug513322-authors

Revision history for this message
Vincent Ladeuil (vila) wrote :

It's easier to review changes if you just push to your original branch and put the mp status back to 'Needs review', your additional revisions then appear after the review comments.

There are still a few details though:

91 + def authors(self, rev, who, short=False, sep=None):
92 + if self._author_list_handler is not None:
93 + author_list_handler = self._author_list_handler
94 + else:
95 + author_list_handler = author_list_registry.get(who)

If 'who' changes, your cached value for the handler is wrong, I don't think it
worth taking the risk, get rid of the cached value instead.

PEP8 nits:

133 +def author_list_all(rev):
134 + return rev.get_apparent_authors()[:]
135 +
136 +def author_list_first(rev):
137 + lst = rev.get_apparent_authors()
138 + try:
139 + return [lst[0]]
140 + except IndexError:
141 + return []
142 +
143 +def author_list_committer(rev):

Two blank lines between functions.

This could be fixed by the second reviewer before merging I think.

review: Needs Fixing

« Back to merge proposal