On Thu, Jul 02, 2009 at 03:42:56PM +0100, Dave Page wrote:
> On Thu, Jul 2, 2009 at 3:22 PM, Joshua Tolley<eggyknap@gmail.com> wrote:
> > On Thu, Jul 02, 2009 at 08:41:27AM +0100, Dave Page wrote:
> >> As far as I'm aware, there's been no code
> >> review yet either, which would probably be a good idea.
> >
> > I don't have loads of time in the coming days, but IIRC I've taken a glance at
> > a past version of the code, and would be willing to do so again, if it would
> > be useful.
>
> If you can look over it, that would be great. i'm not really qualified
> to review perl code, and we always prefer to have at least two sets of
> eyeballs on anything that we put into production for obvious reasons.
On the assumption that other folks' testing has included bug hunting and the
like, I've spent the review time I was able to muster up figuring out the code
and looking for stuff that scared me. I didn't find anything that jumped out.
I did wonder if the %ACTION hash in Handler.pm ought not perhaps include a
flag to indicate that that action needs authentication, and have the handler
take care of that instead of the individual actions. Perhaps a similar
technique could be profitably employed for the titles. Oh, and in Patch.pm,
s/with/which in "patches with have been Committed".
Finally, I ran Perl::Critic, and attached an (admittedly untested) patch to
clean up the things it whined about.
--
Joshua Tolley / eggyknap
End Point Corporation
http://www.endpoint.com