Subject: Re:  New: Put user config options at top of Conf.pm

> Basically, most users will only ever change a small number of options.
> So instead of having them spread throughout the M::SA::Conf POD docs,
> let's put the most common one up at the top of the Conf.pm file.  That
> way if they're looking for "score", or a bayes tweak or something, it
> won't require going through a bunch of options they either can't use
> or otherwise don't care about (or otherwise shouldn't be touching! ;)

I think we should perhaps separate the documentation somewhat from the
implementation.  Perhaps make Conf.pm just the parser and reader and put
basic options into "Basic", and advanced options into "Advanced".

Also, moving large blocks of stuff around in Conf.pm seems a bit
wasteful, I'd still like us to do something like Malte's reorganization
first (proposed some time ago).  The parsing could be made significantly
faster by hashing on the first word in the line instead of doing a
series of regexp if-then-elses.

Daniel

Subject: Re:  Put user config options at top of Conf.pm

> Also, moving large blocks of stuff around in Conf.pm seems a bit
> wasteful, I'd still like us to do something like Malte's reorganization
> first (proposed some time ago).  The parsing could be made significantly
> faster by hashing on the first word in the line instead of doing a
> series of regexp if-then-elses.

Ah, great, I see you got some of the benefit by just switching to "eq"
operators.  There's still the possible benefit of modularizing and
separating by file, but there's definitely less need now.

I wouldn't mind a discussion to see if we can agree on which options are
basic, though.

Daniel

"I wouldn't mind a discussion to see if we can agree on which options are
basic, though."

Sure.  To start: options that users are allowed to use via spamd.  Things like 
score, whitelist*, blacklist*...

The goal being that users shouldn't have to go digging through the Conf doc to 
get to the options they're likely going to use.

Subject: Re:  Put user config options at top of Conf.pm

> Sure.  To start: options that users are allowed to use via spamd.
> Things like score, whitelist*, blacklist*...

I'd add these to that list:

  ok_languages
  ok_locales
  required_hits
  report_safe (but not the add/remove header stuff)

I exposed each of those in the SAproxy GUI which is a reasonable
benchmark for basic options.  :-)

I also had an option to turn off network tests, but we don't have a
global option for that except in spamassassin/spamd (where it does *not*
belong, it should be just another configuration option).

I suggest leaving off autolearning and bayes options since a site
administrator is likely to set those, or the user is quickly headed for
advanced territory if they start mucking with them.

And The rest are not basic...

agreed that local/net should be in Conf, not on the commandline btw.

IMO, a good way to do this would be to separate the code from that gigantic if {
} if { } if { } block, by doing something like this:

  =item POD for option

  blah blah

  =back

  { 
      setting => 'required_score',
      priv => 0,
      dflt => 5,
      validate => \&is_integer,
      code => sub {
        my ($self, $setting, $val, $line) = @_;
        $self->{$setting} = $val;
      }
  },

  [ ... other settings... ]


Pros:

- Since the "priv" member would model if the setting is privileged or not, we
can then move them around without worrying about that.

- Since the string "setting" is static, we can very efficiently parse the config
by doing string compares on that instead of REs.

- There's no function call overhead until the correct entry has been found.

- we can move the default setting to that block, near its POD doc.

- we could even split it into multiple POD files/modules if that is desirable.
(I think it may be, to be honest; one for Bayes, one for defining rules, etc.)

- note that 'validate' provides a way to share validation code.

- note that the "code" block above is highly generic and could probably be the
default for most settings, something like how 'validate' is dealt with.

However, we should probably still special-case the settings ("body", "describe"
etc.) that occur most frequently, for speed.

--j.

I'm assuming the deafening silence on this means everyone likes the idea ;)

actually, another point-- this method would allow modules to have their own
config, so we could

a) have plugins just register their config with Conf at startup, and have the
config more cleanly handled on the plugins, rather than the current
parse_config() callback, which is slower;

b) have the more esoteric Bayes parameters on Bayes.pm itself instead of
cluttering up Conf.

also there's a lot more code-sharing this way... I think this is a winner.

Subject: Re:  Put user config options at top of Conf.pm

On Tue, May 25, 2004 at 08:23:32PM -0700, bugzilla-daemon@bugzilla.spamassassin.org wrote:
> also there's a lot more code-sharing this way... I think this is a winner.

Sure, make it so. ;)

Created attachment 1971 [details]
first draft

OK, here's the first draft; not checking in just yet, this is just for people
to look at ;)

: jm 1137...; diffstat ~/ftp/p
 Conf.pm | 2206
+++++++++++++++++++++++++++++++++++++---------------------------
 1 files changed, 1292 insertions(+), 914 deletions(-)

runtimes:

0m9.009s = 10 runs of "spamassassin -L --lint" with new code
0m9.240s = 10 runs of "spamassassin -L --lint" with old code

checked in as 20526.  we now just need to reorder the Conf items in Conf.pm to
close off this bug ;)

right, that's the reordering taken care of... done! 20563.