Skip to content

Migrate CLI argument parsing from builder to derive API - #1024

Open
Wilfred wants to merge 4 commits into
masterfrom
claude/clap-options-parsing-0uerxj
Open

Migrate CLI argument parsing from builder to derive API#1024
Wilfred wants to merge 4 commits into
masterfrom
claude/clap-options-parsing-0uerxj

Conversation

@Wilfred

@Wilfred Wilfred commented Aug 3, 2026

Copy link
Copy Markdown
Owner

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, and ArgAction) to the derive API (using the Parser derive macro). This modernizes the codebase and makes argument definitions more declarative and easier to maintain.

Key Changes

  • Migrated to clap derive API: Replaced manual app() function that built arguments with a #[derive(Parser)] struct Args with field-level attribute macros
  • Improved error handling: Created dedicated error reporting functions (arg_error, bad_arguments) that use clap's error formatting for consistency
  • Refactored argument parsing logic:
    • Extracted parse_language_override() and parse_glob() as value parsers
    • Created parse_numbered_env_vars() to handle numbered environment variables (DFT_OVERRIDE_1 through DFT_OVERRIDE_9)
    • Extracted combine_overrides() to group adjacent overrides by language
  • Added new types:
    • OnOff enum with ValueEnum derive for on/off style arguments
    • LanguageOverrideArg type alias for parsed override values
    • PathArgs enum to represent different calling conventions
  • Extracted path parsing: Created parse_paths() function to interpret positional arguments according to supported calling conventions
  • Enhanced validation: Improved error messages for invalid arguments, including better reporting of which environment variable caused an error
  • Updated dependencies: Added derive feature to clap in Cargo.toml
  • Added ValueEnum derives: Applied to ColorOutput, DisplayMode, and BackgroundColor for clap integration

Notable Implementation Details

  • The Args struct uses #[command(...)] attributes to configure the command itself (name, version, help text, etc.)
  • Field-level doc comments serve as help text with verbatim_doc_comment to preserve formatting
  • The after_help() function was extracted to provide the examples shown at the end of --help
  • Error handling now uses clap's ErrorKind enum for proper error categorization
  • The refactoring maintains backward compatibility with all existing calling conventions and environment variables
  • Added comprehensive test coverage for new error cases (invalid overrides, wrong number of arguments, etc.)

https://claude.ai/code/session_0162QcLTys6tv1s1Lg1jw2i5

claude added 4 commits August 2, 2026 14:45
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
@joyously

joyously commented Aug 3, 2026

Copy link
Copy Markdown

Do the tests cover absolute versus relative paths, different separators for Windows, and UTF8 characters in filenames?

@Wilfred

Wilfred commented Aug 6, 2026

Copy link
Copy Markdown
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?

@joyously

joyously commented Aug 6, 2026 via email

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants