[m-rev.] for review: improve usage and version messages in the slice directory

[Julien Fischer]

On Sun, 1 Oct 2023, Zoltan Somogyi wrote:

>
> On 2023-10-01 11:55 +11:00 AEDT, "Julien Fischer" <jfischer at opturion.com> wrote:
>> @@ -44,67 +47,146 @@ main(!IO) :-
>>      getopt.process_options(OptionOps, Args0, Args, GetoptResult),
>>      (

...

>>          GetoptResult = ok(OptionTable),
>> +        ( if lookup_bool_option(OptionTable, help, yes) then
>> +            long_usage(StdOutStream, !IO)
>> +        else if lookup_bool_option(OptionTable, version, yes) then
>> +            display_version(StdOutStream, !IO)
>> +        else
>> +            main_2(StdOutStream, OptionTable, Args, !IO)
>>          )
>
> I would leave the switch on the length of Args here, and call
> what is now main_2 with PassFileName and FailFileName.

Done.

> And I would replace the name main_2 with a name such as
> compute_and_output_dice.

Done.

>> +long_usage(OutStream, !IO) :-
>> +    io.write_string(OutStream, "Name: mdice - Mercury dice tool\n", !IO),
>> +    write_copyright_notice(OutStream, !IO),
>> +    io.write_string(OutStream, "\nUsage: mdice [<options>] <passfile> <failfile>\n", !IO),
>> +    io.write_string(OutStream, "\nDescription:\n", !IO),
>> +    io.write_prefixed_lines(OutStream, "\t", [
>> +        "`mdice' creates a comparison between passing and failing runs of a",
>> +        "program."
>> +    ], !IO),
>> +    io.write_string(OutStream, "\nArguments:\n", !IO),
>>      io.write_string(OutStream,
>> -        "Usage: mdice [-s sortspec] [-m module] [-l N] [-n N] [-p N] [-f N] "
>> -        ++ "passfile failfile\n",
>> -        !IO).
>> +        "\tArguments are assumed to Mercury trace count files.\n", !IO),
>> +    io.write_string(OutStream, "\nOptions:\n", !IO),
>> +    io.write_prefixed_lines(OutStream, "\t", [
>> +        "-?, -h, --help",
>> +        "\tPrint help about using mdice (on the standard output) and exit",
>> +        "\twithout doing any further processing.",
>> +        "--version",
>> +        "\tPrint version information.",
>> +        "-s <sortspec>, --sort <sortspec>",
>> +        "\tSpecify how the output should be sorted.",
>> +        "\t(See the Mercury User's Guide for details.)",
>> +        "-l <N>, --limit <N>",
>> +        "\tLimit the output to at most N lines.",
>> +        "-m <module>, --module <module>",
>> +        "\tRestrict the output to the given module and its submodules (if any).",
>> +        "-n <N>, --max-column-name <N>",
>
> The code of long_option recognizes max-name-column, not max-column-name.

Fixed.

...

>>  :- pred long_option(string::in, option::out) is semidet.
>>
>> -long_option("sort",             sort).
>> -long_option("limit",            max_row).
>> -long_option("max-name-column",  max_pred_column).
>> -long_option("max-path-column",  max_path_column).
>> -long_option("max-file-column",  max_file_column).
>> -long_option("module",           modulename).
>> +long_option("help",            help).
>> +long_option("version",         version).
>> +long_option("sort",            sort).
>> +long_option("limit",           max_row).
>> +long_option("max-name-column", max_pred_column).
>> +long_option("max-path-column", max_path_column).
>> +long_option("max-file-column", max_file_column).
>> +long_option("module",          modulename).
>
> The naming scheme for the column width options here is consistent,
> but it is nevertheless bad, for two reasons. First, --max-name-column
> should correspond to max_name_column, not max_pred_column.
> Second, neither the internal nor the external name is really descriptive
> without the word "width" being there somewhere. I would add "_width"
> to the end of both the internal and external names, keeping the
> existing external names for backward compatibility.

I will address in a separate change as it will require changes to the
user's guide. (I've put an XXX there for now.)

>> @@ -40,64 +44,144 @@ main(!IO) :-

...

>>          GetoptResult = error(GetoptError),
>>          GetoptErrorMsg = option_error_to_string(GetoptError),
>> -        io.format(StdOutStream, "%s\n", [s(GetoptErrorMsg)], !IO)
>> +        io.format(StdOutStream, "%s\n", [s(GetoptErrorMsg)], !IO),
>> +        io.set_exit_status(1, !IO)
>> +    ).
>
> What I wrote about the switch on Args in mdice.m also applies
> here in mslice.m.

Done.

>>  :- pred long_option(string::in, option::out) is semidet.
>>
>> -long_option("sort",             sort).
>> -long_option("limit",            max_row).
>> -long_option("max-name-column",  max_pred_column).
>> -long_option("max-path-column",  max_path_column).
>> -long_option("max-file-column",  max_file_column).
>> -long_option("module",           modulename).
>> +long_option("help",            help).
>> +long_option("version",         version).
>> +long_option("sort",            sort).
>> +long_option("limit",           max_row).
>> +long_option("max-name-column", max_pred_column).
>> +long_option("max-path-column", max_path_column).
>> +long_option("max-file-column", max_file_column).
>> +long_option("module",          modulename).
>
> And here.

Added an XXX for now; I shall similarly deal with this separately.

>> -:- pred usage(io.text_output_stream::in, io::di, io::uo) is det.
>> +%---------------------------------------------------------------------------%
>>
>> -usage(OutStream, !IO) :-
>> -    io.write_string(OutStream,
>> -        "Usage: mtc_diff -o outputfile tracecountfile1 tracecountfile2\n",
>> -        !IO).
>> +:- pred display_version(io.text_output_stream::in, io::di, io::uo) is det.
>> +
>> +display_version(OutStream, !IO) :-
>> +    Version = library.mercury_version,
>> +    io.format(OutStream, "Mercury trace count diff, version %s",
>> +        [s(Version)], !IO),
>> +    Package = library.package_version,
>> +    ( if Package = "" then
>> +        io.nl(OutStream, !IO)
>> +    else
>> +        io.format(OutStream, " (%s)\n", [s(Package)], !IO)
>> +    ),
>> +    write_copyright_notice(OutStream, !IO).
>> +
>> +:- pred short_usage(io.text_output_stream::in, io::di, io::uo) is det.
>> +
>> +short_usage(OutStream, !IO) :-
>> +    io.progname_base("mtc_diff", ProgName, !IO),
>> +    io.format(OutStream,
>> +        "Usage: %s [<options>] <tracecountfile1> <tracecountfile2>]\n",
>> +        [s(ProgName)], !IO),
>> +    io.format(OutStream, "Use `%s --help' for more information.\n",
>> +        [s(ProgName)], !IO).
>
> Given that mtc_diff refuses to do its job without the option
> that specifies the output file name, I would mention it explicitly
> even in the short usage message.

Done.

I think we should change mtc_diff and mtc_union to write their
output to stdout by default.

>>  :- pred long_option(string::in, option::out) is semidet.
>>
>> -long_option("out",              output_filename).
>> +long_option("help", help).
>> +long_option("version", version).
>> +long_option("out", output_filename).
>
> "out" is not a good *long* option name. I would add "output-file"
> as a synonym, and advertise *that* in the long usage message.

Done.

>> +:- pred short_usage(io.text_output_stream::in, io::di, io::uo) is det.
>> +
>> +short_usage(OutStream, !IO) :-
>> +    io.progname_base("mtc_union", ProgName, !IO),
>> +    io.format(OutStream, "Usage: %s [<options>] [<files>]\n",
>> +        [s(ProgName)], !IO),
>> +    io.format(OutStream, "Use `%s --help' for more information.\n",
>> +        [s(ProgName)], !IO).
>
> Same point as above about the output file option, for mtc_union here.

Done.

>>  :- pred long_option(string::in, option::out) is semidet.
>>
>> +long_option("help",             help).
>> +long_option("version",          version).
>>  long_option("out",              output_filename).
>>  long_option("verbose",          verbose).
>
> And here.

Done.

> The rest is fine.

Thanks for that!

Julien.