Re: svn commit: r467629 - /spamassassin/trunk/lib/Mail/SpamAssassin/Plugin.pm

[Justin Mason <jm...@jmason.org>]

Michael Parker writes:
> Michael Parker wrote:
> > jm@apache.org wrote:
> >> Author: jm
> >> Date: Wed Oct 25 05:57:25 2006
> >> New Revision: 467629
> >>
> >> URL: http://svn.apache.org/viewvc?view=rev&rev=467629
> >> Log:
> >> add public API to allow plugins to add methods to @TEMPORARY_METHODS.  Michael, may need to redo parts of this with backporting from yr branch, but it's required in trunk to fix a test failure here
> > 
> > Hmmm.  So, I made finish_tests a plugin method so that each plugin can
> > handle this stuff on its own.  I'd rather NOT make this a supported
> > public plugin method, in favor of the other.
> > 
> > Thoughts?  Besides, getting off my ass and making the other just work.
> 
> To be clear, I'd rather keep finish_tests method and do away with the
> register_generated_rule_method method.

I was thinking about that -- however, for cleaning up temporary methods,
which almost all rule types will be doing, it seems to be unneccessary
duplication to have each plugin contain this code:

  our @TEMPORARY_METHODS = ();

  sub build_rule_methods_or_whatever {
    ...
    push @TEMPORARY_METHODS,
            "Mail::SpamAssassin::Plugin::RuleType::".$method;
    ...
  }

  sub finish_tests {
    my ($self) = @_;
    foreach my $method (@TEMPORARY_METHODS) {
      if (defined &{$method}) {
        undef &{$method};
      }
    }
  }


for what it's worth, I agree that finish_tests() should be a public API,
*too*, since some rule type plugins may need to do other stuff.  But
since all rule types will duplicate the @TEMPORARY_METHODS code, why not
let the API do the legwork and keep the plugins simpler?

--j.

Re: svn commit: r467629 - /spamassassin/trunk/lib/Mail/SpamAssassin/Plugin.pm

[Michael Parker <pa...@pobox.com>]

Justin Mason wrote:
> Michael Parker writes:
>> Michael Parker wrote:
>>> jm@apache.org wrote:
>>>> Author: jm
>>>> Date: Wed Oct 25 05:57:25 2006
>>>> New Revision: 467629
>>>>
>>>> URL: http://svn.apache.org/viewvc?view=rev&rev=467629
>>>> Log:
>>>> add public API to allow plugins to add methods to @TEMPORARY_METHODS.  Michael, may need to redo parts of this with backporting from yr branch, but it's required in trunk to fix a test failure here
>>> Hmmm.  So, I made finish_tests a plugin method so that each plugin can
>>> handle this stuff on its own.  I'd rather NOT make this a supported
>>> public plugin method, in favor of the other.
>>>
>>> Thoughts?  Besides, getting off my ass and making the other just work.
>> To be clear, I'd rather keep finish_tests method and do away with the
>> register_generated_rule_method method.
> 
> I was thinking about that -- however, for cleaning up temporary methods,
> which almost all rule types will be doing, it seems to be unneccessary
> duplication to have each plugin contain this code:
> 
>   our @TEMPORARY_METHODS = ();
> 
>   sub build_rule_methods_or_whatever {
>     ...
>     push @TEMPORARY_METHODS,
>             "Mail::SpamAssassin::Plugin::RuleType::".$method;
>     ...
>   }
> 
>   sub finish_tests {
>     my ($self) = @_;
>     foreach my $method (@TEMPORARY_METHODS) {
>       if (defined &{$method}) {
>         undef &{$method};
>       }
>     }
>   }
> 
> 
> for what it's worth, I agree that finish_tests() should be a public API,
> *too*, since some rule type plugins may need to do other stuff.  But
> since all rule types will duplicate the @TEMPORARY_METHODS code, why not
> let the API do the legwork and keep the plugins simpler?
> 

But then you have to expose internal workings, for instance:
+sub register_generated_rule_method {
+  my ($self, $nameofsub) = @_;
+  push @Mail::SpamAssassin::PerMsgStatus::TEMPORARY_METHODS,
+        $nameofsub;
+}


Who says that TEMPORARY_METHODS is where method names are stored in the
future?  In fact in the Check plugin branch that variable no long exists.

Each plugin should be responsible to deciding internally how they will
store their generated methods for later cleanup, not make a guess about
where other plugins store them.  That is way too hackish for me.

About as far down this path I'd be willing to go might be a public
method on the PerMsgStatus object, that allowed for the storage of
generated methods, then you could call that instead.  But I see no harm
in letting plugins clean up their own messes, then you don't force
plugins to store their information the same way.

Michael