Re: [PATCH] Improve Configuration Checking and Parsing

Hi, Amos, Alex,

Thanks a lot for your comments! They are really good advice.

The updated patch is attached. It addresses all the issues you
mentioned except whether to deprecate the "enable"/"disable" options,
which is worth of discussion.

Thanks,
Tianyin

On Thu, Jan 10, 2013 at 8:14 AM, Alex Rousskov
<rousskov_at_measurement-factory.com> wrote:
> On 01/05/2013 05:12 PM, Tianyin Xu wrote:
>
>> @@ -193,6 +290,8 @@
>> return false;
>> } else if ((port = strtol(token, &tmp, 10)), !*tmp) {
>> /* port */
>> + port = xatos(token);
>> +
>> } else {
>> host = token;
>> port = 0;
>
> Please do not set "port" twice because it is confusing and the comma
> operator in the expression only makes things worse. Do something like
> this instead:
>
> else if (strtol(token, &tmp, 10) && !*tmp) {
> port = xatos(token);
> }
>
>
>> + debugs(0, DBG_PARSE_NOTE(DBG_IMPORTANT), "WARNING: 'enable' and 'disable' are deprecated. Please update to use value 'on' and 'off'.");
>
> s/value/values/
>
> or simply remove the word "value".
>
> IMO, it would be better to make the above message specific to 'enable'
> or 'disable' case, so that the admin knows which value to grep for in
> the config file.
>
> Have others discussed deprecation of enable/disable? If not, perhaps
> they do not need to be deprecated at all?
>
>
> Thank you,
>
> Alex.
>

--
Tianyin XU,
http://cseweb.ucsd.edu/~tixu/