On Wednesday 21 January 2004 23:58 CET Justin Mason wrote:
> >2. The code is very monolithic and tangled. That makes it hard to extend
> >the app with "plug-ins". I personally (and I think somebody else said so
> >too) would like to see SA not only as "the application" SpamAssassin
> > with its stable codebase but as something like a framework on which
> > other people might easily develop (eg. without reimplementing all that
> > mail-parsing stuff) their own anti-spam solutions. That would also fit
> > together with us becoming an own toplevel project (or what that's
> > called) in the ASF.
>
> Yes! I have been planning to work on this. There's a bug open ;)
I knew there was something. I have 900 unread messages in my SA-Bugs folder,
maybe I had a glimpse at that bug while I was skimming over another 100 a
few days ago :)
> I will post a spec on the proposed plugin API, for eval tests at
> least. I may not be able to include new learner methods in *that*
> API just yet. let's see.
Nice. What I forgot: We need some way to preserve our namespace. If the
people just throw stuff into Rules/ (or wherever), we might get into
trouble. We could reserve some prefix or subdirectory. But I'll wait for
your spec :)
>[...]
> > AutoWhitelist.pm Rules/Autowhateverlist.pm
> > We really need to rename that one :)
>
> not sure if this is still required.
Think so. The name (not of the module but the stuff itself) causes confusion
over and over again. Somebody once suggested AutoGraylist. I also like
something with the word "Balance" in it like Michael suggested.
> > Bayes.pm Learner/Bayes.pm
> > BayesStore.pm Learner/Bayes/Store.pm
> > The above is a factory for the correct
> > Storage module.
> > Learner/Bayes/StoreDBM.pm
> > That's DB_File or whatever we currently
> > use.
> > Learner/Bayes/StoreSQL.pm
> > Not yet available :)
> > Learner/Fitz.pm
> > Just a sample for possible future
> > plug-ins
>
> OK -- one piece of feedback here. Having two levels:
>
> Learner/Bayes.pm
> Learner/Bayes/Foo.pm
>
> I really do *not* like this split-level approach -- that causes lots of
> trouble already with the existing Mail::SpamAssassin class, which is only
> done like that because CPAN requires it.
>
> It should be e.g.
>
> Learner/Bayes/Bayes.pm
> Learner/Bayes/Foo.pm
>
> this is more amenable to grepping and comprehension IMO.
Hmmm... there's no accounting for taste :) I prefer
Foo.pm
Foo/Helper.pm
because to me that's more comprehensible. And you can still 'grep -R Foo*'.
Foo/Foo.pm also looks weird. (What about Foo/This.pm? muhahaha) But in the
end I wouldn't really care :)
> I haven't bothered commenting on the remaining instances of "split-level"
> below, but consider them all objected-to on these grounds ;)
>
> > CmdLearn.pm App/Learner.pm
> > This isn't really a lib but an application
> > of its own. I don't like this, some stuff
> > really ought to go into sa-learn.pl while
> > some "lib" stuff might stay here and
> > shareable code should go under Util/ or so.
>
> the policy was to keep as much perl code as possible outside of the .pl's
> in future.
Why? They are apps, not libs. One (IMHO) good reason why we should not dump
all code into lib/ is that if one wants to split the package (as I planned
for the Gentoo ebuild) into frontends and libs (which are also usable
without the former, see AMaViS), then he has problems. At least should we
try to have several lib/s (if that's possible).
> > Conf.pm Conf.pm
> > The rewritten Conf.pm.
So this would become Conf/Conf.pm?
> > Conf/ConfSQL.pm
> > Conf/Conf*.pm
> > "optional" features/plug-ins should get
> > their own config module which define the
> > options available.
> > Conf/Store.pm
> > A factory for the config storage backend.
> > Conf/StoreFile.pm
> > Implementation of the standard config file
> > format.
> > ConfSourceSQL.pm Conf/StoreSQL.pm
> > And for SQL.
>
> storage? eek.
>
> How will we support storage of Conf settings? we don't yet.
What did you say? :) Do you mean that we don't "store" the conf ourselves
but just read them? Hm. I just wanted to keep the "prefix" in sync with the
"group" (Store/*.pm) of backends which also provides generic function for
accessing that "storage". But I won't fight for that if you like
Conf/SourceSQL.pm or something else better :)
> > DBBasedAddrList.pm Rules/AddrList.pm
> > Anybody got a better name for this?
> > Rules/AddrList/StoreDBM.pm
> > Rules/AddrList/StoreSQL.pm
> > The Backends.
> > Dns.pm Rules/DNSDBs.pm
>
> Rule/DNSBL.pm I suggest.
I went by the argument in the link I gave: We don't only access blacklists
but also whitelists. But actually, we could both have DNSBL.pm and DNSWL.pm
that would make it even clearer.
> An approach: we should avoid plurals if possible.
The "s" in DNSDBs was no plural but a typo btw ;-)
>[...]
> > PerMsgLearner.pm Learner.pm
> > A factory? I'm not sure.
> > PerMsgStatus.pm Status.pm
> > Shorter is better :)
>
> Now -- the issue with the PerMsg prefix is this. Both of these are
> created by the Mail::SpamAssassin factory on a per-message basis.
> I would *strongly* like to keep that clear in the object name.
Ok, that's a point. But it's so annoying having to type tab three times to
edit those files :) Ok, now that PersistantSomething is gone there are only
two tabs left. But I personally don't like abbreviations in module/class
names. Maybe MessageStatus.pm...? Nah, I dunno...
> In fact, IMO we should *not* change this as it's a *very* user-visible
> change and will require code changes on their behalf, for little
> benefit.
Theo already broke BC (at least did I read that from his commit to spamd).
What's even worse than requiring the users to change their code is if the
interface looks like the old one but behaves differently. We should
document each of those changes btw.
> Suggestions?
>
> > PersistentAddrList.pm
> > Dunno. Looks like a factory but don't ask
> > me where to put it.
>
> now that we only support DBBasedAddrList, I don't think we need to
> keep this as a factory interface -- and can drop this class.
I don't even know what those both do :)
> > Received.pm Parser/Trace.pm
> > I think we parse other trace headers (like
> > Delivered-To) here, too.
>
> nope ;) just Received hdrs, so Parser/Received.pm is better.
But we might... ah crap, I don't care :)
> > Reporter.pm
> > Ummm... dunno.
>
> Report/Reporter.pm? and split out the pyzor/razor reporting into
> their own classes.
Reporter/Reporter.pm if we want to stay consistent :) Splitting the stuff is
a good idea.
>[...]
> > $engine->destroy(); # It should be possible to free some mem.
>
> finish(). That's the std we've already chosen, let's not
> change that ;)
Oh. As I said before, some parts of the SA API are still mystery for me.
Cheers,
Malte
--
[SGT] Simon G. Tatham: "How to Report Bugs Effectively"
<http://www.chiark.greenend.org.uk/~sgtatham/bugs.html>
[ESR] Eric S. Raymond: "How To Ask Questions The Smart Way"
<http://www.catb.org/~esr/faqs/smart-questions.html>
