[m-rev.] for review / comment: structured errors for getopt

[Julien Fischer]

On Wed, 26 Mar 2014, Peter Wang wrote:

> On Tue, 25 Mar 2014 17:21:34 +1100 (EST), Julien Fischer <jfischer at opturion.com> wrote:
>>
>> For review / feedback.
>>
>> I'm not intending to push this to github in its current form; in requesting a review
>> I am mainly seeking feedback on the following things:
>>
>> (1) the overall proposal.
>>
>> (2) in particular, the names of the constructors of the option_error/1 type and
>>      their documentation.
>>
>> Before pushing this change (in whatever form it ends up) a corresponding set of
>> changes will be made to library/getopt.m.
>
> Not really related, but I would like to see getopt.m and getopt_io.m
> share an implementation.  Perhaps getopt_io can define a typeclass for
> handling file_special and getopt defines a dummy instance of that just,
> but I haven't looked at it in detail.

The original justification for the two modules (from the log message
from the original commit of getopt_io) was:

     Since processing options may now involve accessing a file, the main
     predicate process_options now needs a pair of I/O state arguments.
     Since some places where one wants to process options do not have I/O
     states, we want to retain the old interface of process_options as well.
     Since it is not practical to parameterize a group of predicates to work
     both in the presence and in the absence of I/O states, I added the code
     in a new module. While most changes to option handling should be done to
     both getopt.m and getopt_io.m, I expect such changes to be few and far
     between, so I think overall the new module is the best solution.

I am inclined to agree with that last bit and suspect that any shared
implementation is probably going to be more trouble than it's worth.

>> diff --git a/library/getopt_io.m b/library/getopt_io.m
>> index 84ea522..69cac88 100644
>> --- a/library/getopt_io.m
>> +++ b/library/getopt_io.m
>> @@ -153,6 +153,27 @@
>>       option_table(OptionType)::in, maybe_option_table(OptionType)::out,
>>       set(OptionType)::out, io::di, io::uo) 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_io.process_options_se(option_ops(OptionType)::in(option_ops),
>> +    list(string)::in, list(string)::out, maybe_option_table_se(OptionType)::out,
>> +    io::di, io::uo) is det.
>> +
>> +:- pred getopt_io.process_options_se(option_ops(OptionType)::in(option_ops),
>> +    list(string)::in, list(string)::out, list(string)::out,
>> +    maybe_option_table_se(OptionType)::out, io::di, io::uo) is det.
>
> Perhaps it should continue option processing after encountering an
> error, so that the program can report multiple errors in a single run?
> Then the output arguments could be:
>
>    list(string)::out, output_table(OptionType)::out,
>    list(option_error(OptionType))::out,
>
> so changing the arity and allowing you to drop the _se suffix ;)

Returning multiple errors occurred to me as well, although I restricted
this change to cover only what the module currently supports.
(Having proper contexts for errors in file_special arguments would be 
nice as well.)

>> @@ -255,12 +276,78 @@
>>       ;       string(string)
>>       ;       maybe_string(maybe(string)).
>>
>> -:- type option_table(OptionType) ==  map(OptionType, option_data).
>> +:- type option_table(OptionType) == map(OptionType, option_data).
>>
>>   :- type maybe_option_table(OptionType)
>>       --->    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(string)
>> +            % An option that is not recognized appeared on the command line.
>> +            % The argument gives the option as it appeared on the command line.
>> +
>> +    % The first two arguments of the remaining constructors of this type
>> +    % identify the option enumeration value and the string that appeared on the
>> +    % command line for that option respectively.
>> +
>> +    ;       unknown_type(OptionType, string)
>> +            % No type has been specified for the option.
>> +
>> +    ;       requires_argument(OptionType, string)
>> +            % The option requires an argument but it occurred on the command
>> +            % line without one.
> ...
>
> I suggest factoring out the first two arguments.
>
>    :- type option_error(OptionType)
> 	--->	unrecognized(string)
> 	;	option_error(OptionType, string, option_error_reason).
>
>    :- type option_error_reason
> 	--->	unknown_type
> 	;	requires_argument
> 	;	does_not_allow_argument(string)
> 	;	...

Ok.

>> @@ -425,8 +548,8 @@ getopt_io.process_arguments([Option | Args0], Args, OptionOps,
>>                   Args = Args0
>>               )
>>           ;
>> -            ErrorMsg = "unrecognized option `-" ++ Option ++ "'",
>> -            Result = error(ErrorMsg),
>> +            Error = unrecognized(Option),
>> +            Result = error(Error),
>>               OptionArgs = OptionArgs0,
>>               Args = Args0
>>           )
>
> I kept forgetting to fix that bug.

To be honest, I'd never even noticed it!

Cheers,
Julien.