[m-rev.] for review: improve usage and version messages in the slice directory
Zoltan Somogyi
zoltan.somogyi at runbox.com
Sun Oct 1 16:09:58 AEDT 2023
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),
> - (
> - Args = [],
> - usage(StdOutStream, !IO)
> - ;
> - Args = [_],
> - usage(StdOutStream, !IO)
> - ;
> - Args = [PassFileName, FailFileName],
> - lookup_string_option(OptionTable, sort, SortStr),
> - lookup_string_option(OptionTable, modulename, Module),
> - lookup_int_option(OptionTable, max_row, MaxRow),
> - lookup_int_option(OptionTable, max_pred_column, MaxPredColumn),
> - lookup_int_option(OptionTable, max_path_column, MaxPathColumn),
> - lookup_int_option(OptionTable, max_file_column, MaxFileColumn),
> - ( if MaxPredColumn = 0 then
> - MaybeMaxPredColumn = no
> - else
> - MaybeMaxPredColumn = yes(MaxPredColumn)
> - ),
> - ( if MaxPathColumn = 0 then
> - MaybeMaxPathColumn = no
> - else
> - MaybeMaxPathColumn = yes(MaxPathColumn)
> - ),
> - ( if MaxFileColumn = 0 then
> - MaybeMaxFileColumn = no
> - else
> - MaybeMaxFileColumn = yes(MaxFileColumn)
> - ),
> - read_dice_to_string(PassFileName, FailFileName, SortStr, MaxRow,
> - MaybeMaxPredColumn, MaybeMaxPathColumn, MaybeMaxFileColumn,
> - Module, DiceStr, Problem, !IO),
> - ( if Problem = "" then
> - io.write_string(StdOutStream, DiceStr, !IO)
> - else
> - io.write_string(StdOutStream, Problem, !IO),
> - io.nl(StdOutStream, !IO),
> - io.set_exit_status(1, !IO)
> - )
> - ;
> - Args = [_, _, _ | _],
> - usage(StdOutStream, !IO)
> + ( 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.
And I would replace the name main_2 with a name such as
compute_and_output_dice.
> +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.
max-name-column wouldn't fit with the names of the related options anyway.
> :- 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.
> @@ -40,64 +44,144 @@ main(!IO) :-
> getopt.process_options(OptionOps, Args0, Args, GetoptResult),
> (
> GetoptResult = ok(OptionTable),
> - (
> - Args = [],
> - usage(StdOutStream, !IO)
> - ;
> - Args = [FileName],
> - lookup_string_option(OptionTable, sort, SortStr),
> - lookup_int_option(OptionTable, max_row, MaxRow),
> - lookup_int_option(OptionTable, max_pred_column, MaxPredColumn),
> - lookup_int_option(OptionTable, max_path_column, MaxPathColumn),
> - lookup_int_option(OptionTable, max_file_column, MaxFileColumn),
> - lookup_string_option(OptionTable, modulename, Module),
> - ( if MaxPredColumn = 0 then
> - MaybeMaxPredColumn = no
> - else
> - MaybeMaxPredColumn = yes(MaxPredColumn)
> - ),
> - ( if MaxPathColumn = 0 then
> - MaybeMaxPathColumn = no
> - else
> - MaybeMaxPathColumn = yes(MaxPathColumn)
> - ),
> - ( if MaxFileColumn = 0 then
> - MaybeMaxFileColumn = no
> - else
> - MaybeMaxFileColumn = yes(MaxFileColumn)
> - ),
> - read_slice_to_string(FileName, SortStr, MaxRow,
> - MaybeMaxPredColumn, MaybeMaxPathColumn, MaybeMaxFileColumn,
> - Module, SliceStr, Problem, !IO),
> - ( if Problem = "" then
> - io.write_string(StdOutStream, SliceStr, !IO)
> - else
> - io.write_string(StdOutStream, Problem, !IO),
> - io.nl(StdOutStream, !IO),
> - io.set_exit_status(1, !IO)
> - )
> - ;
> - Args = [_, _ | _],
> - usage(StdOutStream, !IO)
> + ( 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)
> )
> ;
> 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.
> :- 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.
> @@ -45,81 +48,161 @@ main(!IO) :-
> getopt.process_options(OptionOps, Args0, Args, GetoptResult),
> (
> GetoptResult = ok(OptionTable),
> - lookup_string_option(OptionTable, output_filename, OutputFile),
> + ( 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(StdErrStream, OptionTable, Args, !IO)
> + )
> + ;
> + GetoptResult = error(GetoptError),
> + GetoptErrorMsg = option_error_to_string(GetoptError),
> + io.format(StdErrStream, "%s\n", [s(GetoptErrorMsg)], !IO),
> + io.set_exit_status(1, !IO)
> + ).
> +
> +:- pred main_2(io.text_output_stream::in, option_table::in,
> + list(string)::in, io::di, io::uo) is det.
> +
> +main_2(StdErrStream, OptionTable, Args, !IO) :-
> + lookup_string_option(OptionTable, output_filename, OutputFile),
> + ( if
> + Args = [Arg1, Arg2],
> + OutputFile \= ""
> + then
> + read_trace_counts_source(Arg1, MaybeTraceCounts1, !IO),
> + (
> + MaybeTraceCounts1 = list_ok(_, _)
> + ;
> + MaybeTraceCounts1 = list_error_message(Msg1),
> + io.write_string(StdErrStream, Msg1, !IO),
> + io.nl(StdErrStream, !IO)
> + ),
> + read_trace_counts_source(Arg2, MaybeTraceCounts2, !IO),
> + (
> + MaybeTraceCounts2 = list_ok(_, _)
> + ;
> + MaybeTraceCounts2 = list_error_message(Msg2),
> + io.write_string(StdErrStream, Msg2, !IO),
> + io.nl(StdErrStream, !IO)
> + ),
> ( if
> - Args = [Arg1, Arg2],
> - OutputFile \= ""
> + MaybeTraceCounts1 = list_ok(Type1, TraceCounts1),
> + MaybeTraceCounts2 = list_ok(Type2, TraceCounts2)
> then
> - read_trace_counts_source(Arg1, MaybeTraceCounts1, !IO),
> + diff_trace_counts(TraceCounts1, TraceCounts2, TraceCounts),
> + write_trace_counts_to_file(diff_file(Type1, Type2),
> + TraceCounts, OutputFile, WriteResult, !IO),
> (
> - MaybeTraceCounts1 = list_ok(_, _)
> + WriteResult = ok
> ;
> - MaybeTraceCounts1 = list_error_message(Msg1),
> - io.write_string(StdErrStream, Msg1, !IO),
> - io.nl(StdErrStream, !IO)
> - ),
> - read_trace_counts_source(Arg2, MaybeTraceCounts2, !IO),
> - (
> - MaybeTraceCounts2 = list_ok(_, _)
> - ;
> - MaybeTraceCounts2 = list_error_message(Msg2),
> - io.write_string(StdErrStream, Msg2, !IO),
> - io.nl(StdErrStream, !IO)
> - ),
> - ( if
> - MaybeTraceCounts1 = list_ok(Type1, TraceCounts1),
> - MaybeTraceCounts2 = list_ok(Type2, TraceCounts2)
> - then
> - diff_trace_counts(TraceCounts1, TraceCounts2, TraceCounts),
> - write_trace_counts_to_file(diff_file(Type1, Type2),
> - TraceCounts, OutputFile, WriteResult, !IO),
> - (
> - WriteResult = ok
> - ;
> - WriteResult = error(WriteErrorMsg),
> - io.write_string(StdErrStream, "Error writing to " ++
> - "file `" ++ OutputFile ++ "'" ++ ": " ++
> - string(WriteErrorMsg), !IO),
> - io.nl(StdErrStream, !IO)
> - )
> - else
> - % The error message has already been printed above.
> - true
> + WriteResult = error(WriteErrorMsg),
> + io.format(StdErrStream,
> + "Error writing to file`%s': %s\n",
> + [s(OutputFile), s(io.error_message(WriteErrorMsg))],
> + !IO),
> + io.set_exit_status(1, !IO)
> )
> else
> - usage(StdOutStream, !IO)
> + % The error message has already been printed above.
> + true
> )
> - ;
> - GetoptResult = error(GetoptError),
> - GetoptErrorMsg = option_error_to_string(GetoptError),
> - io.format(StdOutStream, "%s\n", [s(GetoptErrorMsg)], !IO)
> + else
> + short_usage(StdErrStream, !IO),
> + io.set_exit_status(1, !IO)
> ).
That diff of mtc_diff.m is hard to read, because it resynchronized on an "if"
that plays different roles in the before and after versions.
Can you please resend it with -b --patience?
> -:- 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.
> :- 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.
> +:- 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.
> :- 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.
The rest is fine.
Zoltan.
More information about the reviews
mailing list