-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Daniel Quinlan writes:
> [EMAIL PROTECTED] (Justin Mason) writes:
>
> > - (public) message rewriting functionality
> >
> > rewrite_mail
> >
> > move code into another class; leave this public API on the ::Scan
> > object which calls into that class.
> > Proposed class name: Mail::SpamAssassin::Scan::Rewriter?
>
> Rewriting should not be part of the Scan object.
yes, actually, you're right.
> I'd propose that
> rewriting be part of a Mail::SpamAssassin::Format class.
any particular reason for that name?
> > - (internally) methods that implement Eval tests
> >
> > [entire contents of EvalTests.pm which
> > do this horrible hack of putting themselves into
> > the PerMsgStatus namespace]
> >
> > move code into another namespace. Eval tests use the
> > PerMsgStatus object as $self, and since they're just
> > functions, not objects themselves, that doesn't need to
> > change -- they'd still get the ::Scan object as their
> > first arg.
> >
> > Proposed namespace: Mail::SpamAssassin::Test::Eval?
>
> Just Mail::SpamAssassin::Tests.pm ?
yeah, actually, why not.
> > - (internally) methods that control how tests are run,
> > their ordering etc.
> >
> > [parts of check]
> > [parts of do_head_tests / etc. ]
> >
> > Definitely move.
> > Proposed class: Mail::SpamAssassin::TestRunner?
> > RunTests? Runner? Scanner?
>
> Shouldn't this just be part of "Scan"?
This is the thing -- as Theo said, by moving to a new class,
we can provide the ability to switch out implementations without
having to change the class of the "Scan" object (ie. what
gets returned to the user).
Basically the key idea is that we're breaking it up by *what
it does* and what it's semantics are:
- Scan: object returned to user
- [this class]: object that contains the algorithm and code
to run whatever subset of the tests in whatever order
And the idea is that all this logic shouldn't be in the
simple "results" object we give back to the user.
> > - (internally) methods that implement the DNS event-driven algorithm
> >
> > [entire contents of Dns.pm which do this horrible hack of putting
> > themselves into the PerMsgStatus namespace]
> >
> > into Mail::SpamAssassin::TestRunner as above?
>
> I'd say this belongs in the EvalTests module, wherever it ends up.
Hmm. not sure about that.
the EvalTests module can be kept for just the eval tests that
are defined; this is "plumbing". In fact, it's more similar to
the TestRunner chunk imo.
There's about 650 lines of code there, too, which is a lot
(for perl).
> > - (internally) methods that perform auto-learning
> >
> > learn
> >
> > Proposed class: Mail::SpamAssassin::AutoLearn? (I don't think
> > mushing into PerMsgLearner, Bayes, or Mail::SpamAssassin makes
> > sense, so a new class would be better.)
>
> I think there's too much breaking up of stuff here. Bayes would be
> fine.
yeah, OK, Bayes is probably good enough alright.
> Do we really need to do this now? This is not going to significantly
> help performance, accuracy, or memory usage, is it?
> What's the effect on stability?
> How does this affect our release cycle?
ok, ok. it's not much use to any of those -- but the "all mushed into one
class"-ness of PerMsgStatus is really driving me nuts ;) It's far from
good OO design. And bad "code smell" is an indicator that there are
inefficiencies there.
I do have an idea for improving performance -- separate mail to follow.
- --j.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.4 (GNU/Linux)
Comment: Exmh CVS
iD8DBQFBWNeIQTcbUG5Y7woRAljYAJ9jP0fX4MoLlSVzZgmYT8gmylA90wCfZgXw
Zgj4vTKNAwhG6jQL7QAkkPU=
=nPeb
-----END PGP SIGNATURE-----
