nl: match GNU's errors for its numeric and numbering-style option values - #14314
Closed
arbelonson-source wants to merge 1 commit into
Closed
nl: match GNU's errors for its numeric and numbering-style option values#14314arbelonson-source wants to merge 1 commit into
arbelonson-source wants to merge 1 commit into
Conversation
nl's numeric options (-i/--line-increment, -l/--join-blank-lines,
-v/--starting-line-number, -w/--number-width) validated their values
with clap's own i64/u64 value_parser, so:
- a negative value passed as its own argument (`-i -5`, not the
attached `-i-5`) was rejected as an unrecognized flag instead of
being read as the option's value, since these Args did not have
allow_hyphen_values;
- an invalid value produced clap's own generic wording instead of
GNU's (e.g. 'invalid digit found in string' instead of "invalid line
number increment: 'bogus'"), and did not distinguish GNU's two
distinct overflow messages: a value that overflows the *option's
own* underlying integer width (e.g. --number-width is a plain C
int, so anything outside i32 -- even though it parses as an i64 just
fine) is "Value too large for defined data type", while one that
fits that width but falls outside the option's accepted range (e.g.
--number-width also requires a positive value) is "Numerical result
out of range".
Also fixes the header/body/footer numbering style options (-h, -b, -f)
and the number format option (-n), which validated via a plain
value_parser and clap's own PossibleValuesParser respectively, same
underlying wording problem. GNU reports each of these with a
field-specific message ("invalid header numbering style", vs "body"
vs "footer", vs "invalid line numbering format" for -n) that a single
generic message can't produce, appends 'Try --help' (which the numeric
options and an invalid regular expression for -h/-b/-f do not), and --
unlike the numeric options, which stop parsing immediately at the
first bad one -- collects every bad style/format option and reports
them together, in the order they were actually given on the command
line.
Replaced the existing 'Invalid arguments supplied.' preamble + joined
list, which didn't match any of this, with per-error 'nl: ' prefixed
lines (only the first of which the outer error handler adds for us)
and a single trailing hint line when appropriate.
AI-assisted-by: Claude Opus 5, via Claude Code
Contributor
|
You should use |
Contributor
Author
|
Closing proactively — same template as the batch you just closed (#14304, #14312, etc.): reworks clap's already-correct wording to match GNU's text. There's a genuine secondary fix bundled in here too (negative values as their own arg were rejected as unrecognized flags), but given #14304 got closed despite having the same kind of bundled fix, I'll assume the wording-matching part outweighs it. Happy to reopen a hyphen-values-only version if that's wanted separately — just say the word. |
Contributor
|
sure |
|
GNU testsuite comparison: |
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
3 participants
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.
What
nl's numeric options (-i/--line-increment,-l/--join-blank-lines,-v/--starting-line-number,-w/--number-width) validated theirvalues with clap's own
i64/u64value parser, which caused twoseparate problems:
1. A negative value as its own argument was rejected as an unrecognized flag:
(The attached form,
-i-5, already worked -- only the separate-argument form was broken.)2. An invalid value got clap's own generic wording, and conflated two GNU messages that mean different things:
The last two are genuinely different failure modes in GNU's wording
(overflowing the option's own C integer width vs. fitting that width
but failing a separate business-rule check) and needed to stay
distinguished.
Also fixed
The header/body/footer numbering-style options (
-h/-b/-f) andthe number-format option (
-n) have the same underlying "clap's ownwording instead of GNU's" problem (via a plain
value_parserandPossibleValuesParserrespectively). GNU reports each with afield-specific message ("invalid header numbering style" vs "body" vs
"footer", vs "invalid line numbering format" for
-n), appends "Try--help" (which the numeric options and a bad regular expression for
-h/-b/-fdo not), and -- unlike the numeric options, which stopparsing immediately at the first bad one -- collects every bad
style/format option and reports them together, in the order they were
actually given on the command line.
Replaced the existing
parse_optionserror path (a generic "Invalidarguments supplied." preamble + a plain joined list) with GNU's real
shape: per-error
nl:-prefixed lines (only the first of which theouter error handler adds automatically) and a single trailing hint
line only when appropriate.
Testing
cargo test -p uu_nl/ fulltests/by-util/test_nl.rssuite: 69 passed, 0 failed.-iand-v.LC_ALL=C.cargo clippy -p uu_nl --all-targets -- -D warningsandcargo fmt --check: clean.This PR was written with AI assistance (Claude Opus 5, via Claude Code). I've tested the changes but please review the code carefully.