refactor(globals): split parse_args into informational and evaluation modes (#206) - #222
Merged
Conversation
… modes (#206) parse_args accepted -h, -v and -ci in two places with two meanings apiece, and the ambiguity was real only because both meanings could occur in one invocation. Deciding the mode by argc before any flag is read makes them disjoint, so the overload stays and the ambiguity goes. Informational mode, entered when fewer than the three mandatory arguments are present, accepts only -h/--help, -v/--version and -ci/--citation. None takes a value; each prints and exits 0. Anything else, including --verbosity, warns 'Invalid usage.' and prints usage. Evaluation mode accepts every other flag, each consuming exactly one following token, and rejects the three informational flags by name: Option '-h' is informational only; use it without the mandatory arguments rather than the generic "Unexpected option '%s'", which reads as "no such flag" and misdirects a user who has made a scoping mistake rather than a typo. Removing -n/--no-output-files is what makes "every flag takes a value" exceptionless: it was the only main-loop flag that took none. Its one writer gone, g.write would have been permanently true, so the eight always-taken guards, the write_outputs row in parameters.tsv, and b2s() -- which existed for that one call site -- all go with it. The re-indentation from unwrapping those guards dominates the diff. One behavior change beyond the contract, and it is a bug fix. The verbosity pre-pass looped to argc-2, so i could never equal argc and the missing-value ERROR was unreachable: a trailing -v was silently accepted and the parse succeeded with verbosity unchanged. Starting the loop at 4 is correct now that -v is legal only past the positionals, and it makes the guard fire. parameters.tsv loses its write_outputs row. The file is row-keyed rather than columnar, so nothing else shifts; only a consumer reading that key is affected, and the value would document nothing once -n is gone. The archived docs/v2.* trees still describe -n as those releases shipped it and are left untouched. print_usage is the only live documentation of the flag set: it gains a second usage line for the informational form and moves -h/-v/-ci out of Miscellaneous into their own section. 731 unit tests and 98 pytest cases pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
Authorship: the content below was drafted by Claude Opus 5 (an AI coding agent) and
filed via
ghunder @TimD1-bot, a bot account operated by @TimD1. It reflects theagent's analysis, not a statement authored by @TimD1.
Closes #206. Also resolves #70, which recorded the
-voverload as a latent defect and left thedecision open.
Problem
parse_argsaccepted-h,-vand-ciin two places with two meanings apiece. #70 recorded-vas a latent defect — version in theargc < 4pre-check, verbosity in the main option loop —and left open whether to fix the overload or document it.
Neither is quite right. The overload is ambiguous only because both meanings can occur in a single
invocation. Decide the mode by
argcbefore any flag is read and-vis unambiguous everywhereit can appear, so the overload can stay.
Change
Two disjoint invocation modes.
Informational — fewer than the three mandatory arguments. Accepts only
-h/--help,-v/--version,-ci/--citation; none takes a value, each prints and exits 0. Anything elsewarns
Invalid usage., prints usage, and exits 0.Evaluation — all three mandatory arguments present. Every optional flag consumes exactly one
following token. The three informational flags are rejected.
-h,--help-v--version--verbosityInvalid usage.-ci,--citation-n,--no-output-filesInvalid usage.Unexpected optionInvalid usage.The rejection names the token as typed rather than reusing the generic
Unexpected option '%s':-his a real option, so "unexpected" reads as "no such flag" and misdirects a user who has madea scoping mistake rather than a typo.
-nremoved-n/--no-output-fileswas the only main-loop flag taking no value, so removing it is what makesthe one-value rule exceptionless. It was
g.write's only writer, against nine readers, so thefield would have been permanently
true. All of it goes: the eight always-taken guards(
main.cpp×2,print.cpp×6), thewrite_outputsrow inparameters.tsv, andb2s(), whichexisted solely for that row. Re-indentation from unwrapping those guards dominates the diff.
Verbosity pre-pass bug fix
One behavior change beyond the contract. The pre-pass loop header was
for (int i = 0; i+1 < argc; i++), soinever exceededargc-2,i == argccould not hold, and the missing-valueERRORwas unreachable — a trailing
-vwas silently accepted and the parse succeeded with verbosityunchanged. Starting at 4 is correct now that
-vis legal only past the positionals, and it makesthe guard fire.
ParseArgs.VerbosityMissingNotReached, which documented the dead path rather thanenforcing it, becomes
ParseArgs.VerbosityMissingErrors.User-visible surface
parameters.tsvloses itswrite_outputsrow. The file is row-keyed rather than columnar, sonothing else shifts; only a consumer reading that key is affected, and the value documents nothing
once the flag is gone.
print_usagegains a second usage line for the informational form and moves-h/-v/-ciout ofMiscellaneous into an "Informational (use without arguments)" section.
The archived
docs/v2.3.3,v2.3.4,v2.4.0andv2.5.3trees each document-nas thatrelease shipped it and are untouched — editing them would misrepresent those releases. There is
no current or v3 tree, so
print_usageis the only live documentation of the flag set; the removalgets reflected when the v3 docs are written under #52.
Tests
Builds on the
parse_argscoverage added in #202.ParseArgs.NoOutput→NoOutputRejected(informational mode) andNoOutputMainLoopErrors(evaluation mode); the
g.writeassertion goes with the fieldHelpMainLoop,VersionMainLoop,CitationMainLoopflip from asserting printed output toasserting the informational-only
ERRORand exit 1, joined byHelpLongMainLoopandCitationLongMainLoopVerbosityMissingNotReached→VerbosityMissingErrors, asserting the now-reachable errorVerbosityLongRejectedpins--verbosityas invalid in informational modeDashVMeansVersionOrVerbosityByModepins the-vpair as one disjoint-mode contract ratherthan two unrelated behaviors
PrintUsage.ListsDocumentedFlagsdrops-n, --no-output-files;PrintUsage.RequiredSectiongains the second usage line
-nonly incidentally and were retargeted:OptionalBeforeMandatoryWarns(neededany flag token before the positionals) and
VerbositySkippedInMainLoop(needed a trailing flagthe main loop consumes; now
-q 5)parse_capturing_stdoutis deleted — the three main-loop cases were its only callers, and anunused helper in an anonymous namespace warns
Verification
and the
-vpair case added,NoOutputremoved).devbase. The four that differ areaccounted for:
parameters.tsv(the droppedwrite_outputs truerow, plus thecommandrowrecording each binary's own path),
summary.vcf(##CL=only, same reason),runtime.tsv(wall-clock), and stderr (log timestamps and durations — identical once normalized). Both sides
built with
make cleanin between.match, including the exit codes.
dist.cpp:1141: unused parameter 'thread2', present on thebase as well.
Notes
Based on current
dev, which already includes #202'sparse_argstests.The informational-only rejection fires in the main option loop, so it lands after the VCFs are
opened and the reference FASTA is loaded — same ordering the pre-existing
Unexpected optionpathhas always had. Erroring earlier would mean a second scan of
argvbefore the positionals arevalidated, which is a larger restructuring than this issue scopes.