Re: First CommitFest: July 15th

On Wed, Jul 8, 2009 at 1:11 AM, Joshua Tolley<eggyknap(at)gmail(dot)com> wrote:
> 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(at)gmail(dot)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.

Possibly so. We may also find that it needs to be a bit more
fine-grained than that, as there are already three levels of access
(public, login required, administrator login required) and there could
theoretically be more in the future.

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

Fixed, thanks.

> Finally, I ran Perl::Critic, and attached an (admittedly untested) patch to
> clean up the things it whined about.

As usual, I'm unimpressed by the whining emitted by Perl::Critic. I
can understand that if a function is really intended to return void
(but perl doesn't have that concept) then you probably ought to write
just "return" rather than "return undef". But if the function
sometimes returns a value and sometimes returns "undef", insisting
that the word "undef" not be spelled out explicitly seems pretty
silly.

The other changes have marginally more merit, though some of them
break with surrounding whitespace conventions.

...Robert