Code review comment for lp:~benji/launchpad/bug-597324

On Tue, Jul 20, 2010 at 5:33 AM, Robert Collins
<email address hidden> wrote:
> Review: Needs Fixing code
> test_redirect_logic looks like it has 3 or 4 separate tests in it -
> please split that out for readability.

Done. They somehow seem less readable though. And the constants that
they use now have greater scope than before. Perhaps these should be
in their own TestCase instead. Yeah, that's what I'll do.

> Your fakes could do with a docstring each to describe their intent,

Added.

> and lastly your static methods are a little weird; if they are not
> stateful, please consider functions; if they are tightly coupled, but
> are called from instances, please use normal methods.

They are not stateful, but are very tightly related to the class in
question and called from instances thereof.

I prefer methods that don't use self to be static, but am glad to honor
local custom. I removed the staticmethod decorator and added self to
the parameter list.

> With these changes it can land without another review.

Land ho!
--
Benji York