Migrate CLI argument parsing from builder to derive API - #1024
Open
Wilfred wants to merge 4 commits into
Open
Conversation
Replace the hand-written `Command` builder in options.rs with a declarative `Args` struct. The `--help` and `-h` output is unchanged. Values that previously required manual validation after parsing are now parsed by clap: * `--display`, `--color`, `--background`, `--syntax-highlight` and `--strip-cr` use `ValueEnum`, so there are no `unreachable!` branches for values clap has already checked. * `--override` and `--override-binary` use a `value_parser`, so invalid globs and unknown language names are reported as clap errors that say which value was rejected. The numbered `DFT_OVERRIDE_N` and `DFT_OVERRIDE_BINARY_N` environment variables can't be expressed declaratively, so they're still read after parsing, but they now use the same value parsers and report errors in the same style, naming the environment variable at fault. Difftastic supports several calling conventions (2 paths, a single file with conflict markers, or the 7 and 9 argument forms used by GIT_EXTERNAL_DIFF), so the positional arguments are still matched on afterwards. That logic now lives in `parse_paths`, which returns a `PathArgs` enum rather than a six-element tuple. Errors on misuse are more helpful: * Being given an unsupported number of arguments now lists all the calling conventions supported. * Being given options but no paths at all previously exited silently with code 2, and now explains the problem. * The "no conflict markers" error now names the file, and no longer hand-rolls its own usage output. All of these still exit with EXIT_BAD_ARGUMENTS. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0162QcLTys6tv1s1Lg1jw2i5
Doc comments read better than `help = "..."` attributes, and they show
up in rustdoc too. They're all `verbatim_doc_comment`, so clap doesn't
reflow the shell examples in --override and --override-binary, and
doesn't strip the trailing full stops.
Since clap treats the first paragraph of a doc comment as the short
help, `-h` is now much shorter (62 lines rather than 124) whilst
`--help` is unchanged. The only wording change is that --override and
--override-binary now have "For example:" in a paragraph of its own, so
that their short help doesn't end with a dangling colon.
Doc comments can't use env!("CARGO_BIN_NAME"), so the examples hardcode
the binary name. Added a test that this matches.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0162QcLTys6tv1s1Lg1jw2i5
Master switched from crossterm's IsTty to std::io::IsTerminal, which conflicted with the import block in options.rs. Kept both changes: the new clap derive imports and std::io::IsTerminal. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0162QcLTys6tv1s1Lg1jw2i5
Reverted the incidental changes, so the diff is closer to just replacing the builder with the derive API: * Reverted the CHANGELOG. * Removed the unit tests. The CLI tests cover the same behaviour. * Restored the original match on the positional arguments, rather than introducing a PathArgs enum. * Removed the DEFAULT_CONTEXT_LINES and MAX_NUMBERED_ENV_VAR constants, and the LanguageOverrideArg type alias. * Merged arg_error and bad_arguments into a single function, so we no longer thread an ErrorKind through the call sites. * Simplified the message for an unsupported number of arguments: it no longer special cases zero arguments, and it shows the whole invocation rather than reconstructing it from the paths. * Moved DisplayOptions and DiffOptions back to their original position in the file. Also trimmed the CLI tests down to the calling conventions and the error paths that changed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0162QcLTys6tv1s1Lg1jw2i5
|
Do the tests cover absolute versus relative paths, different separators for Windows, and UTF8 characters in filenames? |
Owner
Author
|
@joyously I was exploring using derive because it's more concise and allows me to use doc comments for argument descriptions, but I don't think it's worth it after playing with this code. The explicit API is much easier to reason about for a CLI with multiple usage conventions. It has found some interesting argument bugs though. Regarding your concerns about funky paths, have you seen any issues with current difftastic? |
|
No I have not used difftastic, because it's so large, and I have not been coding much...
But I have been following the Jujutsu project which uses `clap`, and they have had all the issues I mentioned.
|
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.
This PR refactors the command-line argument parsing in difftastic to use clap's derive API instead of the builder API, improving code maintainability and consistency.
Summary
The argument parsing logic has been migrated from clap's builder pattern (using
Command,Arg, andArgAction) to the derive API (using theParserderive macro). This modernizes the codebase and makes argument definitions more declarative and easier to maintain.Key Changes
app()function that built arguments with a#[derive(Parser)]structArgswith field-level attribute macrosarg_error,bad_arguments) that use clap's error formatting for consistencyparse_language_override()andparse_glob()as value parsersparse_numbered_env_vars()to handle numbered environment variables (DFT_OVERRIDE_1 through DFT_OVERRIDE_9)combine_overrides()to group adjacent overrides by languageOnOffenum withValueEnumderive for on/off style argumentsLanguageOverrideArgtype alias for parsed override valuesPathArgsenum to represent different calling conventionsparse_paths()function to interpret positional arguments according to supported calling conventionsderivefeature to clap in Cargo.tomlValueEnumderives: Applied toColorOutput,DisplayMode, andBackgroundColorfor clap integrationNotable Implementation Details
Argsstruct uses#[command(...)]attributes to configure the command itself (name, version, help text, etc.)verbatim_doc_commentto preserve formattingafter_help()function was extracted to provide the examples shown at the end of--helpErrorKindenum for proper error categorizationhttps://claude.ai/code/session_0162QcLTys6tv1s1Lg1jw2i5