Michael Lohmann <mi.al.lohmann@xxxxxxxxx> writes:
> struct option base_options[] = {
> - OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), 'q'),
> - OPT_CMDMODE(0, "continue", &cmd, N_("resume revert or cherry-pick sequence"), 'c'),
> - OPT_CMDMODE(0, "abort", &cmd, N_("cancel revert or cherry-pick sequence"), 'a'),
> - OPT_CMDMODE(0, "skip", &cmd, N_("skip current commit and continue"), 's'),
> + OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), ACTION_QUIT),
> + OPT_CMDMODE(0, "continue", &cmd, N_("resume revert or cherry-pick sequence"), ACTION_CONTINUE),
> + OPT_CMDMODE(0, "abort", &cmd, N_("cancel revert or cherry-pick sequence"), ACTION_ABORT),
> + OPT_CMDMODE(0, "skip", &cmd, N_("skip current commit and continue"), ACTION_SKIP),
> OPT_CLEANUP(&cleanup_arg),
> OPT_BOOL('n', "no-commit", &opts->no_commit, N_("don't automatically commit")),
> OPT_BOOL('e', "edit", &opts->edit, N_("edit the commit message")),
I actually do not terribly mind reusing the single letter option
(e.g. 'x' in "-x") and the command mode, as it is descriptive
enough. The argument that an enum allows compilers warn about
non-exhausitve switches is valid if we have such a switch.
> + switch (cmd) {
> + case ACTION_START:
> + goto skip_opt_compatibility_verification;
> + case ACTION_QUIT:
> + this_operation = "--quit";
> + break;
> + case ACTION_CONTINUE:
> + this_operation = "--continue";
> + break;
> + case ACTION_SKIP:
> + this_operation = "--skip";
> + break;
> + case ACTION_ABORT:
> + this_operation = "--abort";
> + break;
> }
>
> + verify_opt_compatible(me, this_operation,
And indeed the if/elseif cascade in the original is easier to ensure
its exhaustiveness by turning it into a switch.
HOWEVER.
If I were writing this patch, I would rather do it like so:
this_operation = NULL;
switch (cmd) {
case 'q':
this_operation = '--quit";
break;
...
case 'a':
this_operation = '--abort";
break;
default: /* everything else is compatible with others */
break;
}
if (this_operation)
verify_opt_compatible(me, this_operation, ...);
Use of enum is optional; I just didn't like too much churning to
illustrate a single idea (here, the single idea being "switch is
more appropriate then if/else cascade in this case"), and I think
this is easier to read than with enums [*].
Side note: After all the single letter option names are
meant to be mnemonic. "case 'q'" is just as descriptive as
"case ACTION_QUIT" in the context of this switch statement.
HTH.
