[m-rev.] for review / comment: structured errors for getopt
Peter Wang
novalazy at gmail.com
Wed Mar 26 16:32:16 AEDT 2014
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
More information about the reviews
mailing list