Created attachment 2280 [details]
suggested patch

instead of just falling through, this patch makes another goto call (sorry)
past the plugin attempt if ret fails.  I also added in some comments...

-1, I'm not keen on that.  the error message

    invalid value for "add_header": blarg

is a lot more informative as to what the problem really was than

    Failed to parse line in SpamAssassin configuration, skipping: add_header blarg

A better fix would be to keep the "invalid value" error message, then skip to
failed_line, bypassing try_plugin as in your patch.

--j.

Created attachment 2281 [details]
uses MISSING_REQUIRED_VALUE, nicer errors

ok, here's an implementation of what I was suggesting.	this patch also
contains the fix for bug 3699, since they're inseparable.

so, for the "empty description" error case, you now get:

config: SpamAssassin failed to parse line, no value provided for "describe",
skipping: describe FOO
lint: 1 issues detected.  please rerun with debug enabled for more information.


the "add_header" case:

config: SpamAssassin failed to parse line, "blarg" is not valid for
"add_header", skipping: add_header blarg
lint: 1 issues detected.  please rerun with debug enabled for more information.

and an unrecognised command:

config: SpamAssassin failed to parse line, skipping: blarg
lint: 1 issues detected.  please rerun with debug enabled for more information.


note

(a) the use of a scoping "facility" name, as per Daniel's suggestions --
"config" ("lint" may be more appropriate? can be changed on checkin)

(b) clear error messages for all three cases

(c) consistency; previous code would output "warn"s for invalid values, whereas
an unrecognized command in the conf file would be a "dbg"!  This now uses the
same output code for all 3 error cases so it's consistent (dbg, or warn if
--lint is being used).

+1, sure. :)

+0.9, two issues: 
 
1. Why is there a string compare via eq done instead of ==?  Both $ret and 
$INVALID_VALUE should be numbers and the string compare needs a "cast" of the 
numeric value to string first. 
 
2. I guess this one falls under "big API change for which its too late" ;)  But 
I don't like the fact that the success return value for the $cmd->{code}s is 
obviously undef.  That should be either 1 or something like $VALID_VALUE (== 
1); maybe <=0 -> error and >=1 -> success. 
 
btw, if I write +0.9 it means as much as "feel free to round this to +1 and 
commit, I just have some nitpicking issues with the code" :) 

Subject: Re:  [review] Conf::Parser double counts certain errors, etc.

On Mon, Aug 23, 2004 at 10:15:36AM -0700, bugzilla-daemon@bugzilla.spamassassin.org wrote:
> 1. Why is there a string compare via eq done instead of ==?  Both $ret and 
> $INVALID_VALUE should be numbers and the string compare needs a "cast" of the 
> numeric value to string first. 

Because the return value, currently, can be anything, not just a number.
For instance, the hash_key return value is either a key=>value pair or
a number (for invalid/missing).  Some things return undef, some things return
strings, etc.

> 2. I guess this one falls under "big API change for which its too late" ;)  But 
> I don't like the fact that the success return value for the $cmd->{code}s is 
> obviously undef.  That should be either 1 or something like $VALID_VALUE (== 
> 1); maybe <=0 -> error and >=1 -> success. 

Yeah, I was thinking about that as I went through for the first patch.
Hence my comment above the code call.  It should be a bit better defined,
but yeah, that'll be 3.1 I think.  It requires modifying large chunks
of code in Conf/Conf::Parser to return numbers only and not undef
anywhere....

'Because the return value, currently, can be anything, not just a number.
For instance, the hash_key return value is either a key=>value pair or
a number (for invalid/missing).  Some things return undef, some things return
strings, etc.'

and treating it as a number when it could be a string results in horrible
warnings when "use strict" is on.   annoying...

'> 2. I guess this one falls under "big API change for which its too late" ;)  But 
> I don't like the fact that the success return value for the $cmd->{code}s is 
> obviously undef.  That should be either 1 or something like $VALID_VALUE (== 
> 1); maybe <=0 -> error and >=1 -> success. 

Yeah, I was thinking about that as I went through for the first patch.
Hence my comment above the code call.  It should be a bit better defined,
but yeah, that'll be 3.1 I think.  It requires modifying large chunks
of code in Conf/Conf::Parser to return numbers only and not undef
anywhere....'

the success return value could be anything.  it's only the failure cases that we
need to care about.

this was explicit (although I should document it ;).  I'll do that post-checkin
as a doc change.

reason why: there's lots of those 'code' subs, and adding "boilerplate" items to
them, like a required "return 1" line in each one, will just increase code size,
make it more annoying to write those subs, cause errors when people forget that,
and generally cause pain.   dealing with error return codes can be done more
easily in one place (Parser) rather than in 100 places (each of the code subs).
I'm not sure I like it -- 

(where did that "I'm not sure I like it --" come from?! anyway, ignore that ;)

+1

thx, applied, r36836