Bug 3117 - Merge CmdLearn.pm and sa-learn together
Summary: Merge CmdLearn.pm and sa-learn together
Status: RESOLVED FIXED
Alias: None
Product: Spamassassin
Classification: Unclassified
Component: Libraries (show other bugs)
Version: SVN Trunk (Latest Devel Version)
Hardware: Other other
: P5 normal
Target Milestone: 3.0.0
Assignee: SpamAssassin Developer Mailing List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2004-03-01 10:35 UTC by Theo Van Dinter
Modified: 2004-03-01 09:58 UTC (History)
0 users


Attachment Type Modified Status Actions Submitter/CLA Status
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Theo Van Dinter 2004-03-01 10:35:38 UTC 
So I was noticing that the only thing we have that calls CmdLearn is sa-learn, so why have CmdLearn as 
a module?

The alternate view is: Razor does this with their code (razor-check/razor-admin/etc are all shells that 
call into Razor2::Client modules), and it's an absolutely crappy-ass backward design IMNSHO.

Unless anyone has an issue, I'll merge it on Wednesday (3/3).
Comment 1 Duncan Findlay 2004-03-01 10:47:04 UTC 
Subject: Re:  New: Merge CmdLearn.pm and sa-learn together

+1
Comment 2 Michael Parker 2004-03-01 10:49:50 UTC 
Subject: Re:  New: Merge CmdLearn.pm and sa-learn together

+1
Comment 3 Justin Mason 2004-03-01 10:59:53 UTC 
+1 here too.

I originally designed it that way so that sa-learn script was just a tiny call
into the modules, and all the "real code" was in the Mail::SA module space;
seemed likely to be cleaner.

But over time, it's become clear that was wrong -- it was less "clean".  In
particular, the breaking of the connection between option processing and POD
docs is very messy.
Comment 4 Daniel Quinlan 2004-03-01 11:02:07 UTC 
-1

Some of the options might need to be in sa-learn, but we still need a module to
handle learning if we're going to add learning to spamd or if anyone wants to
integrate learning into another application.
Comment 5 Justin Mason 2004-03-01 11:08:48 UTC 
'Some of the options might need to be in sa-learn, but we still need a module to
handle learning if we're going to add learning to spamd or if anyone wants to
integrate learning into another application.'

Dan, we already do with the M:SA:learn() and M:SA:PerMsgLearner apis and
modules.  CmdLearn is only used as the core of the command-line script, makes
assumptions about ARGV, STDIN, etc.  it's pure command-line stuff.
Comment 6 Daniel Quinlan 2004-03-01 13:55:59 UTC 
Subject: Re:  Merge CmdLearn.pm and sa-learn together

> Dan, we already do with the M:SA:learn() and M:SA:PerMsgLearner apis and
> modules.  CmdLearn is only used as the core of the command-line script, makes
> assumptions about ARGV, STDIN, etc.  it's pure command-line stuff.

Hmmm... okay, most of it is options handling, yes, but are you sure you
want to move all of the code in &wanted and the ArchiveIterator code
into sa-learn?

I'm not exactly sure those APIs are ones we want to expose as they're
currently designed and written.
Comment 7 Justin Mason 2004-03-01 14:29:50 UTC 
'Hmmm... okay, most of it is options handling, yes, but are you sure you
want to move all of the code in &wanted and the ArchiveIterator code
into sa-learn?'

ArchiveIterator, no.   &wanted, yes.

ArchiveIterator is a shared utility class used to implement iteration over a
collection of messages, and should remain in Mail::SA::ArchiveIterator.  I doubt
theo was thinking that.
Comment 8 Daniel Quinlan 2004-03-01 14:43:09 UTC 
Subject: Re:  Merge CmdLearn.pm and sa-learn together

> ArchiveIterator, no.   &wanted, yes.

No, I meant the code in CmdLearn.pm that calls ArchiveIterator.  I
suppose it could all be moved over...

I withdraw my previous vote, although I still have a bad feeling about
moving all the code into sa-learn.  Moving the options makes total
sense, of course.

Daniel
Comment 9 Theo Van Dinter 2004-03-01 15:07:20 UTC 
yeah, ArchiveIterator would stay where it is ... it's used for sa-learn, mass-check, and (if I/someone else 
gets time) spamassassin.  definitely staying seperate.

but yeah, CmdLearn is just command options and a bunch of stuff that is _only_ used by sa-learn.  it 
just doesn't make any sense to me to have sa-learn call CmdLearn when nothing else is using it.

Since there are 4 +1s on this now (mine included), I'll go ahead.
Comment 10 Theo Van Dinter 2004-03-01 18:58:08 UTC 
ok, committed to head.  r6960.