[jifty-devel] Janitorial: lib/Jifty/Request.pm

[Jesse Vincent]

On Mar 3, 2007, at 4:19 AM, Edgar Whipple wrote:

> I did some janitorial work on Request.pm, and attached the resulting
> patch for review. I'll wait for comment until Sunday evening before
> committing, or longer on request.


I've done a preliminary review.   If you're going to be doing  
reformatting, it probably makes sense to
use perltidy for everything we can.   I've pasted my .perltidyrc (aka  
the Jifty-canonical .perltidyrc ;) at t
he end of this meesage. At the same time, we're fairly near the merge  
of the T::D branch to trunk, so it
  might make sense to wait until then before a major tidying run.




>
> I did this work against the template-declare branch, because it had  
> the
> most recent version of Request.pm in it.
>
> My primary goal was visual formatting, so I avoided code changes and
> only changed whitespace...with the following exceptions:
>
> -- changed '\|' to '[|]' and '\?' to '[?]' in regexes.

Hm. I don't actually find that more readable, but I'm flexible. Other  
folks?


> -- changed grep from expression form to block form
>    (added {} around expression, removed comma).

Depending on the case, I'm ambivalent.

> -- added explicit 'return' to final expression in subs.

In trivial subs, I'd usually leave this off, but for something  
complex, that seems reasonable

> -- changed 'return undef' to bare 'return'.

Ok. I know it's bad style, but I prefer "return undef". I should  
probably be broken of this preference. (IE, I don't think there's a  
need to go wioth my preference)


> -- in Jifty::Request->clone(), refactored clone creation out of  
> 'bless'
>    call into separate 'my $new_object', for clarity.

I don't actually find the temporary object to be a win, though better  
formatting is probably worthwhile



> -- transferred 'use strict' and 'use warnings' to the inside of each
>    package. (Those pragmas are lexically scoped.)


Keeping them outside the package declaration means they're going to  
act on the whole file, which is a win.
There's no need to repeat them in every package in a file. (And doing  
so means there's a greater chance we'll
miss one when adding a new package.


>
> I did *not* add an explicit return at the end of J::R->delete() or
> J::R->do_mapping(). Their final values were not clear to me, as one  
> has
> nested if's and the other ends in a for loop. Probably an explicit
> return should be bare, but I couldn't be certain without a much closer
> scrutiny.
>

Understood. If you need help untangling, I'm game

>
> Edgar
> -- 
> ln -s /dev/null ~/.sig
> <request.pm-janitorial.patch>
> _______________________________________________
> jifty-devel mailing list
> jifty-devel at lists.jifty.org
> http://lists.jifty.org/cgi-bin/mailman/listinfo/jifty-devel




pinglin:~/svk jesse$ more ~/.perltidyrc
-l=78
-i=4
-ci=4
-se
-vt=2
-cti=0
-pt=1
-bt=1
-sbt=1
-bbt=1
-nsfs
-nolq
-ce
-wbb="% + - * / x != == >= <= =~ !~ < > | & >= < = **= += *= &= <<=  
&&= -= /=
|= >>= ||= .= %= ^= x="

-------------- next part --------------
A non-text attachment was scrubbed...
Name: PGP.sig
Type: application/pgp-signature
Size: 186 bytes
Desc: This is a digitally signed message part
Url : http://lists.bestpractical.com/pipermail/jifty-devel/attachments/20070303/98cd19e7/PGP.pgp