[m-rev.] for review: structured errors for getopt and getopt_io

[Julien Fischer]

On Sun, 15 Jun 2014, Paul Bone wrote:

> On Tue, Jun 10, 2014 at 02:20:17PM +1000, Julien Fischer wrote:
>> diff --git a/library/getopt.m b/library/getopt.m
>> index 6287e3b..9fc647a 100644
>> --- a/library/getopt.m
>> +++ b/library/getopt.m
>> @@ -148,6 +148,27 @@
>>      option_table(OptionType)::in, maybe_option_table(OptionType)::out,
>>      set(OptionType)::out) is det.
>>
>> +% Variants of the above that return structured errors.
>> +% These behave as the above versions except that any error values returned are
>> +% members of the option_error/1 type rather than strings.
>> +
>> +:- pred getopt.process_options_se(option_ops(OptionType)::in(option_ops),
>> +    list(string)::in, list(string)::out, maybe_option_table_se(OptionType)::out)
>> +    is det.
>> +
>> +:- pred getopt.process_options_se(option_ops(OptionType)::in(option_ops),
>> +    list(string)::in, list(string)::out, list(string)::out,
>> +    maybe_option_table_se(OptionType)::out) is det.
>> +
>> +% getopt.process_options_track_se(OptionOps, Args, OptionArgs,
>> +%       NonOptionArgs, OptionTable0, Result, OptionsSet)
>> +
>
> What's this commented out code for?

It's not commented out code; it's the docuemntation for
process_option_track_se.  (There's a similar thing for
process_options_track above -- arguably the documentation should be
better, but I don't intend to address that in this change.)

>
>> +:- pred getopt.process_options_track_se(
>> +    option_ops_track(OptionType)::in(option_ops_track),
>> +    list(string)::in, list(string)::out, list(string)::out,
>> +    option_table(OptionType)::in, maybe_option_table_se(OptionType)::out,
>> +    set(OptionType)::out) is det.
>> +
>>  :- pred init_option_table(
>>      pred(OptionType, option_data)::in(pred(out, out) is nondet),
>>      option_table(OptionType)::out) is det.
>> @@ -255,6 +276,59 @@
>>      --->    ok(option_table(OptionType))
>>      ;       error(string).
>>
>> +:- type maybe_option_table_se(OptionType)
>> +    --->    ok(option_table(OptionType))
>> +    ;       error(option_error(OptionType)).
>> +
>> +:- type option_error(OptionType)
>> +    --->    unrecognized_option(string)
>> +            % An option that is not recognized appeared on the command line.
>> +            % The argument gives the option as it appeared on the command line.
>> +
>> +    ;       option_error(OptionType, string, option_error_reason).
>> +            % An error occurred with a specific option.  The first two arguments
>> +            % identify the option enumeration value and the string that appeared
>> +            % on the command line for that option respectively.  The third
>> +            % argument describes the nature of the error with that option.
>> +
>
> These lines and others are too long.  (Yes the standard is currently 79 but
> I'd prefer to make it 76 please).

Fixed spots where they are over 79 characters.

>
>> +:- type option_error_reason
>> +    --->    unknown_type
>> +            % No type for this option has been specified in the
>> +            % `option_default'/2 predicate.
>> +
>> +    ;       requires_argument
>> +            % The option requires an argument but it occurred on the command
>> +            % line without one.
>> +
>> +    ;       does_not_allow_argument(string)
>> +            % The option does not allow an argument but it was provided with
>> +            % one on the command line.
>> +            % The argument gives the contents of the argument position
>> +            % on the command line.
>> +
>> +    ;       cannot_negate
>> +            % The option cannot be negated but its negated form appeared on
>> +            % the command line.
>> +
>> +    ;       special_handler_failed
>> +            % The special option handler predicate for the option failed.
>> +
>> +    ;       special_handler_missing
>> +            % A special option handler predicate was not provided for the option.
>> +
>> +    ;       special_handler_error(string)
>> +            % The special option handler predicate for the option returned an
>> +            % error.
>> +            % The argument is a string describing the error.
>> +
>> +    ;       requires_numeric_argument(string).
>> +            % The option requires a numeric argument but it occurred on the
>> +            % command line with a non-numeric argument.
>> +            % The third argument gives the argument as it appeared on
>> the +            % command line.
>> +
>
> third argument?

Fixed.  That was a remanant of an earlier version of this change.
(Before I addressed the previous round of review comments.)

>> diff --git a/library/getopt_io.m b/library/getopt_io.m
>> index 84ea522..d4f54f7 100644
>> --- a/library/getopt_io.m
>> +++ b/library/getopt_io.m
>
> Some (maybe all) of the problems above also occur in this file.

Also, fixed.

>
> The implementation itself is fine, the only problems superficial.

Thanks,

Cheers,
Julien.