Simplify dynamic clap configuration in benchmark_runner cli#8
Open
alamb wants to merge 1 commit into
Open
Conversation
Collapse the per-suite help-visibility machinery into a single command builder. The `SelectorSuiteOptions` / `SelectorOptionVisibility` / `SelectorCommand` types and the `build_cli_for_args_with_options` -> `build_cli_with_help_options` -> `build_cli_with_selector_options` chain existed only to narrow `info <suite> --help` / `query <suite> --help` to the selected suite's options. That narrowing is redundant with the richer curated `info <suite>` view rendered manually in output.rs, and the narrowing behavior was untested. Now `build_cli` registers the union of all suites' options once under a single `Suite Options` heading. Suite-defined options are still registered dynamically with clap, and multi-character short aliases (e.g. `-sf`) are still normalized to their long form before parsing. The only behavior change: `info <suite> --help` / `query <suite> --help` list the union of suite options rather than just the selected suite's. The curated `info <suite>` output is unchanged. Net: removes 3 types and several builder/helper functions (~180 fewer lines in cli.rs). Binary builds, clippy is clean, and all benchmark_runner tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
alamb
commented
May 26, 2026
|
|
||
| /// Suite-option visibility used while constructing selector subcommands. | ||
| #[derive(Clone, Copy)] | ||
| enum SelectorSuiteOptions<'a> { |
Author
There was a problem hiding this comment.
there was a lot of extra runaround to avoid printing a few help items -- I think keeping the code simpler will be good
| pub suite_options: BTreeMap<String, String>, | ||
| } | ||
|
|
||
| /// Per-subcommand suite-option visibility used for help rendering. |
Author
There was a problem hiding this comment.
this is a lot of code that just seems to set a field on visibility 🤔
alamb
force-pushed
the
alamb/simplify-benchmark-runner-cli
branch
from
cddaf45 to
0dbdcd5
Compare
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
1 participant
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.
Which issue does this PR close?
benchmark_runnerinfo and query commands apache/datafusion#22032Rationale for this change
The dynamic clap configuration in
benchmarks/src/benchmark_runner/cli.rswas confusing to me and I found I could simplify it with fewer levels of indirectionHere is a proposal for your consideration