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

[Peter Wang]

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.

> 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 ;)

> @@ -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)
	;	...

> @@ -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.

Peter