perl-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ryan Gies <r...@livesite.net>
Subject Re: [Patch] Apache2::Reload
Date Mon, 31 Aug 2009 12:38:10 GMT
On Fri, 28 Aug 2009 21:35:50 -0400
Philippe wrote:

> On 28/08/09 08:35 , Ryan Gies wrote:
> > This patch for Apache2::Reload does two things:
> > 
> >   A) ignores require-hooks which exist in %INC. (fix)
> 
> That's a very good fix indeed, and should be fixed by itself, yes.
> 
> >   B) reloads by file, not module name (fix?)
> 
> Good feature, good patch. I've got only one simple style nit with it.
> 
> Instead of:
> 
> +    foreach my $kvpair (@changed) {
> +        my $name = $ReloadByModuleName ? $kvpair->[0] : $kvpair->[1];
> +        require $name;
> 
> 
> I just find $kvpair and $kvpair->[0] somewhat hard to read, could you
> simply change it to something like:
> 
> +    foreach my $change (@changed) {
> +        my $module = $change->[0];
> +        my $file = $change->[1];
> +        my $name = $ReloadByModuleName ? $module: $file;
> +        require $name;

Agreed. I used this style presuming it's equally readable:

        my ($module, $file) = @$change;

> 
> Apart from that, it's a great patch!
> 
> Could you resend 2 patches (splitting A) and B) in different ones)
> with that small concern adressed ?
> 

No problem.  I created patch-B as a diff from patch-A, i.e., it's a 
cumulative patch.  I also bumped $VERSION once for each patch.  I
have only ran tests against the final v0.12 code.

I also introduced a new change in patch-B.  The debug output when an
entry is reloaded uses the format:

  Apache2::Reload: process %d reloading %s from %s\n

the last %s *was* populated with the file's full path.  It is *now*
populated with whatever value was passed to 'require'.  For example, if
you are using the old-style (ReloadByModuleName) method, it will look
like this:

  ... from Apache2/Trace.pm

and by default (new-style) it will look like:

  ... from /usr/local/src/lsn/perl/lib/Apache2/Trace.pm

Please let me know if you have any other changes, questions, or
comments! Cheers,
-Ryan
Mime
View raw message