feat: add --version flag with app-injected release string - #2
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2 +/- ##
==========================================
+ Coverage 90.59% 91.10% +0.51%
==========================================
Files 6 6
Lines 999 1068 +69
==========================================
+ Hits 905 973 +68
- Misses 94 95 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds a reusable --version flag to the green::params::params argument parser, allowing applications to inject a release string and have the parser print it and exit successfully when requested.
Changes:
- Registers a new
--versionflag inparamsand addsset_version()/version()APIs. - Implements version print-and-exit behavior during parsing.
- Adds unit tests for the version string round-trip and the
--versionexit behavior (via fork/pipe capture).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/green/params/params.h |
Adds the --version flag registration, version storage/accessors, and version handling in parse(). |
test/params_test.cpp |
Adds tests for version setter/getter and end-to-end --version output/exit behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
9ae7091 to
1721aef
Compare
1721aef to
a5da5eb
Compare
|
Wasn't happy with initial implementation. Now updated and cleaner. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
src/green/params/params.h:312
parse(const std::string&)allocatesargvviaget_argc_argv()(which usesnew[]) but never frees it, leaking memory on every call (including in tests that parse repeatedly). Free the allocated array afterparse(argc, argv)returns.
bool parse(const std::string& s) {
std::string to_parse = s;
auto [argc, argv] = get_argc_argv(to_parse);
return parse(argc, argv);
}
| parsed_ = true; | ||
| if (parameters_map_.empty() && argc > 2) | ||
| return false; // we provided command line parameters but haven't defined any them yet | ||
| bool help_requested = build(); | ||
| return !help_requested; | ||
| build(); | ||
| return !help_requested() && !version_requested(); |
| /** | ||
| * Print the informational message that made parse() return false: the release string for | ||
| * --version, otherwise the help text. Call this on the rank that owns console output. Future | ||
| * informational flags should be handled here so callers can stay agnostic to the specific flag. | ||
| */ | ||
| void help_or_version() { | ||
| if (version_requested()) { | ||
| std::cout << version_ << std::endl; | ||
| return; |
| if (_help || _version) { | ||
| // help(); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
This looks like it's over-engineering. But fine, we can do it.
egull
left a comment
There was a problem hiding this comment.
Fine. Lots of engineering for a bit of parameter parsing, but it works.
| if (_help || _version) { | ||
| // help(); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
This looks like it's over-engineering. But fine, we can do it.
…tor) Add a first-class --version flag as a sibling of --help: - argparse: add a native `version` flag with help_requested()/version_requested() accessors; build() treats --help/--version uniformly (skip validation). - params: inject the version string via the constructor (default ""), delegate flag state to argparse, and report informational flags by returning false from parse() (no std::exit; the caller prints and finalizes). help_or_version() prints the version string or the help text; print()/help() emit a "Version:" line when a version is set. define() rejects the reserved names help/?/version so a consumer parameter can't shadow the built-in flags. - tests: cover constructor injection, --help/--version symmetry, help_or_version output, the Version line in print()/help(), and reserved-name rejection. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
a5da5eb to
efdb55d
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/green/params/params.h:317
parse(const std::string&)leaks theargvarray allocated byget_argc_argv(). This can accumulate in long-running processes or in repeated in-process parses/tests. Use RAII (ordelete[]) to ensure the allocatedchar**is freed even ifparse(argc, argv)throws.
bool parse(const std::string& s) {
std::string to_parse = s;
auto [argc, argv] = get_argc_argv(to_parse);
return parse(argc, argv);
}
Summary
Add a first-class
--versionflag to the params parser, as a true sibling of--help.versionis a native argparse flag (added next to the built-in_helpinlibs/argparse/argparse.h), surfaced viahelp_requested()/version_requested()accessors. No params-layer bolt-on, no cached pointer, no raw-argv scan.paramsconstructor (default"").--versionis command-line only (like--help); it is not INI-settable.parse()returnsfalsefor an informational flag (--helpor--version) — a stop signal. The library does not callstd::exit(); the caller prints the message (on its chosen rank) and finalizes/exits. This suits the MPI programs that embed the parser.help_or_version()prints the release string for--version, otherwise the help text.print()/help()also emit aVersion: <value>line when a version is set.define()rejects the reserved nameshelp/?/version, so a consumer parameter cannot silently shadow the built-in flags.API
Tests
In-process (no fork needed, since nothing exits): constructor injection round-trip,
--help/--versionsymmetry via the accessors,help_or_version()output, theVersion:line inprint()/help(),--versionalongside other args, and reserved-name rejection.🤖 Generated with Claude Code