Re: [PATCH] : Inversion for options

Hi Guys,

On Wed, Dec 26, 2007 at 12:27:52AM +0100, Krzysztof Oledzki wrote:
>
>
> On Wed, 26 Dec 2007, Aleksandar Lazic wrote:
>
> >Hi,
> >
> >On Die 25.12.2007 04:49, Krzysztof Oledzki wrote:
> >
> >>diff --git a/include/common/cfgparse.h b/include/common/cfgparse.h
> >
> >[snipp]
> >
> >>-int cfg_parse_listen(const char *file, int linenum, char **args)
> >>+int cfg_parse_listen(const char *file, int linenum, char **args, int inv)
> >>{
> >> static struct proxy *curproxy = NULL;
> >> struct server *newsrv = NULL;
> >>@@ -1145,8 +1145,8 @@ int cfg_parse_listen(const char *file, int linenum,
> >>char **args)
> >> else if (!strcmp(args[0], "option")) {
> >> int optnum;
> >>
> >>- if (*(args[1]) == 0) {
> >>- Alert("parsing [%s:%d] : '%s' expects an option
> >>name.\n", file, linenum, args[0]);
> >>+ if (*(args[1]) == '\0') {
> >>+ Alert("parsing [%s:%d]: expected option name.\n",
> >>file, linenum);
> >> return -1;
> >> }
> >>
> >
> >I think the %s will help to know which command expected a option, IMHO.
>
> Thank you for checking my patch, however I still think that this %s is
> useless here as there is only one value it can take: "option".
>
> --- cut here ---
> else if (!strcmp(args[0], "option")) {
> int optnum;
>
> if (*(args[1]) == '\0') {
> Alert("parsing [%s:%d]: expected option name.\n", file,
> linenum);
> return -1;
> }
> --- cut here ---
>
> So IMHO there is no point in "parsing [haproxy.cfg:66]: 'option' expects
> an option name.", but if you insist I can keep it unchanged. ;)

Personally, I would prefer to see the offending keyword repeated. Right now, you know that it is the "option" keyword which might be causing trouble because you're working on it, but for someone who writes a configuration from scratch and who encounters errors, seeing "expected option name" does not necessarily imply that the problem is on an "option" line.

Also, "'option' expects an option name" tells something which looks like a rule and the person reading this error will remind exactly that. Without 'option', nothing says if the error is just in the context or is more general.

That said, I've looked at your patch, and it looks good to me. I would just have two very minor comments :

• in error messages reported to the user, it is said that option xxx does not support inversion. I'd prefer to say that "option xxx cannot be negated".
• you'll find me very annoying on this one, but please do not forget to add a space after language keywords such as "for" :-) In fact, I find that it makes it really easier to read the code at certain moments in the night when you know that isolated words are for the language and that words immediately followed by a parenthesis are function names.

If you don't mind, I'll adjust those little changes while applying the patch.

Thanks Krzysztof,
Willy Received on 2007/12/26 10:04