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

Revision history for this message
Martin von Gagern (gagern) 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.

I thought so at first as well. But with the predecessor of this branch
here, I had waited in the "Needs review" state (which the merge requests
maintained anyway, so there was no need to reset anything) for five days
without another vote, then I resubmitted and received an approval within
the day and got it merged within two days.
See https://code.launchpad.net/~gagern/bzr/bug513322-first/+merge/22433
So from that experience, I derived the mental note "resubmit if you want
to get things moving". Nevertheless, I'll give the simple push another try.

> 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.

self._author_list_handler isn't a cache, it's a user-configured
override. The idea is that the log formatter has a preference of whom to
list as author(s), and expresses this preference in the 'who' argument.
The user, on the other hand, can force a different choice, which
overrides whatever the formatter uses by default.
self._author_list_handler stores the handler configured by the user.

Or were you talking about some other cache? If so, I don't see which.

> PEP8 nits:
> Two blank lines between functions.

Fixed and pushed.

« Back to merge proposal