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

[Zoltan Somogyi]

2020-10-09 15:08 GMT+11:00 "Peter Wang" <novalazy at gmail.com>:
>> +report_special_type_mismatch(IsFirst, MismatchSpecial) = Pieces :-
>> +    (
>> +        IsFirst = is_first,
>> +        ReasonIsPieces =
>> +            [words("One possible reason for the error is that")]
>> +    ;
>> +        IsFirst = is_not_first,
>> +        ReasonIsPieces =
>> +            [words("Another possible reason for the error is that")]
>> +    ),
>> +    (
>> +        MismatchSpecial = type_mismatch_special_getopt_error(GetoptModule),
>> +        Pieces = ReasonIsPieces ++
>> +            [words("the signatures of the option processing predicates"),
>> +            words("in the"), quote(GetoptModule), words("module"),
>> +            words("have changed recently."),
>> +            words("Errors are now returned in a structured form,"),
>> +            words("which can be converted to a string by calling the"),
>> +            quote("option_error_to_string"), words("function."), nl]
>> +    ).
> 
> Don't need the parentheses.

I know. They are there to tell readers that this is supposed to be a switch
on the kinds of special mismatches; it just happens that at the moment,
the switch has only one arm.

>> +% The reading of the file obviously requires doing I/O, which means that
>> +% only the predicate variants that take an I/O state pair of arguments
>> +% support file_special options. If a call to a predicate variant that
>> +% does not take a pair of I/O states as argument does nevertheless specify
> 
> Fix "argument", or delete "as argument".

I did the latter.
 
>> +% a file_special option, that predicate will generate an error message
>> +% for that option.
> 
> Perhaps simply:
> 
>     then an error will be reported.

I went with "will report an error for that option".

>> +    % All these predicates place all the non-option arguments in
>> +    % 'NonOptionArgs', and the predicates that have an `OptionArgs' argument
>> +    % place the option arguments there. (While some callers will want
>> +    % the arguments contain the options, other callers will not, considering
>> +    % that the only information they want from them is that contained in
>> +    % the option table.)
> 
> I think we should deprecate
> 
>     process_options(OptionOps, Args, NonOptionArgs, Result)
> 
> and don't introduce
> 
>     process_options_io(OptionOps, Args, NonOptionArgs, Result, !IO)

The predicate now named process_options_io already existed before this diff,
as getopt_io.process_options. The predicate now named process_options
was its analogue in the getopt module.

These two predicates are cheap (their definitions consist of just one call each)
and represent a common use case, so I don't see any reason to deprecate them.

>> +    % `Result' an `ok' wrapped around an option table, which maps each option
>> +    % to its final value. This will be its default value as updated by
>> +    % all the options in `Args'.
>>      %
> 
> The mention of "default values" being "updated" was confusing to me
> (of course the intended meaning can be grasped eventually).
> Maybe replace the last part with something like:
> 
>     which maps each option to a value, as set by the options in `Args'.

I replaced the sentence with

Unless updated by an option in `Args', this will be its default value.

>>      % the initial contents of the option table, instead of calling
>> -    % the initialization predicate itself. The point of this is that
>> +    % the initialization predicate themselves. The point of this is that
>>      % it allows the option table to be initialized once (using either
> 
> I suggest replacing:
> 
>     The point of this is that it allows ...
> 
> with:
> 
>     It allows the option table ...

I have rewritten the whole sentence.

>> -    % Second, each call to process_options_track returns the set of options
>> -    % that were set by that call. This helps with the same objective;
>> -    % for example, it can tell the caller whether an option was set
>> -    % from a configuration file, the command line, both, or neither.
>> +    % Second, each call to these predicates returns the set of options
> 
> return

No, return (singular) is correct here. I added "one of" before "these predicates"
for emphasis.

>> @@ -580,8 +552,18 @@
>>  
>>  %---------------------------------------------------------------------------%
>>  
>> +    % Convert our structured representation of an error to an error message.
>> +    %
>>  :- func option_error_to_string(option_error(OptionType)) = string.
> 
> s/our/from the/

I did s/our/the/

>> +:- func decode_maybe_option_table(maybe_option_table_se(OptionType))
>> +    = maybe_option_table(OptionType).
>> +
> 
> "decode" seems a strange name for this function, but I can only suggest
> "convert_to_maybe_option_table".

It was a strange name for me as well, but couldn't think of a better one.
I went with the one you just proposed.

I followed all your other suggestions.

Thanks for the review.

Zoltan.