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

[Jesse Vincent]

On Mar 3, 2007, at 5:15 PM, Edgar Whipple wrote:

> Jesse,
>
>
> On Sat, 2007-03-03 at 10:06 +0000, Jesse Vincent wrote:
>>> On Mar 3, 2007, at 4:19 AM, Edgar Whipple wrote:
> [snip]
>> 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.
> Agreed on all points.
>
> Your .perltidyrc is a subset of what I used. I would recommend using
> yours for any automated tidy pass, with one addition. I propose adding
> -csc (Closing Side Comments). This annotates the closing brace of a
> large block. I did not use it in Request.pm, because it's a
> non-whitespace change. However, as it helps readability *and* it's
> guaranteed semantically harmless, I strongly recommend it for the
> future.
>
> It works like so:
>
>     sub foo {
>         # ... more than 6 lines will trigger CSC
>     } ## end sub foo
>       ^^^^^^^^^^^^^^ comment automatically added
>

Nope. Absolutely not. That's what your text editor is for.


> Last tidy point: I'll insert your .perltidyrc in the Style.pod once I
> get my notes together (see separate post on Style Questions). If no
> objection, I'll also add a suggestion about coders using format- 
> skipping
> tags (#<<<, #>>>) to preserve manual layout in small areas.

I'd rather review the output of tidying than pollute the source with  
these



>>> -- changed '\|' to '[|]' and '\?' to '[?]' in regexes.
>>
>> Hm. I don't actually find that more readable, but I'm flexible. Other
>> folks?
>
> I suspect I may be better qualified, due to proximity, to  
> anticipate how
> an inexperienced Perl-er will view a given snippet of code :-). Trust
> me, this is less confusing to a newcomer's eye. (theDamian said so  
> too,
> so nyaah!)

I'm willing to try it, but as always, reserve the right to pretend I  
never said it was ok and change it back.

>>> -- added explicit 'return' to final expression in subs.
>>
>> In trivial subs, I'd usually leave this off, but for something
>> complex, that seems reasonable
>
> An instance of defensive programming. Any given trivial sub has a  
> finite
> non-zero probability of becoming non-trivial in the future. The  
> problem
> is, you can't tell in advance which subs are which.

Nope. Not for now.
>

>>> -- 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)
>
> Bad Perl hacker! Bad! No XP for you!
>
> You can bet that someday, someone will inappropriately call your  
> sub in
> list context. BOOM! Bigger badness, very hard to trace. It's enough  
> the
> poor slobs have to troubleshoot their broken code; don't throw in an
> erroneous data value to make it worse.
>
> Come to think of it, perhaps a good item on the janitorial list  
> would be
> identifying the scalar-only subs and marking them with 'return  
> scalar'.
>

"return scalar"?
>>>

>>> -- 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
>>
>
> Again, please trust my young and innocent eyes. This is better for the
> new reader.

Please trust my old and experienced hand. I don't like it.

>>>

>>> -- 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.
>>
>
> Yeah, I wasn't sure about this one. I'll back it out.
>
> Help me understand, though. I thought that a package declaration
> terminated file scope. In other words, that unlike (unpackaged) blocks
> and subs, the contents of a package are not a sub-scope of the file's
> scope. Wrong?


Run this in a file: :)


use warnings;
use strict;
package A;
$x++;
package B;
$y++;
1;





-------------- 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/7ae3379a/PGP.pgp