Repository navigation
[code-reusability] Reuse positive integer helper - #340
Merged
Pedro Henrique Penna (ppenna) merged 1 commit intoOct 4, 2026
Conversation
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The partial callable regresses malformed-integer CLI diagnostics by exposing an unstable implementation repr.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Reuses the shared positive-integer parser while retaining benchmark-specific validation text.
Changes:
- Adds customizable error text to
common.positive_int. - Replaces benchmark validation with a configured shared helper.
- Tests custom validation messages.
| File | Description |
|---|---|
scripts/nvx_tools/common.py |
Adds keyword-only custom error text. |
scripts/nvx_tools/benchmark.py |
Reuses the shared helper. |
scripts/test_nvx_tools.py |
Tests custom error text. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Pedro Henrique Penna (ppenna)
marked this pull request as ready for review
October 4, 2026 14:34
This was referenced Oct 4, 2026
The benchmark parser kept its own positive_int(), which converted the argument with int() and rejected values below 1. common.positive_int() already does the same for the performance, microVM test, and adversarial parsers, but rejects with "must be greater than zero", while the benchmark options report "must be at least 1". Give common.positive_int() a keyword-only message argument, whose default keeps the existing message for those callers, and make the benchmark positive_int() call it with "must be at least 1". Keep the benchmark parser a named function rather than a functools.partial. argparse names the type callable in its "invalid <type> value" error, and a partial has no __name__, so a non-integer value would be reported with the partial's repr, which includes a process-specific function address, instead of as an "invalid positive_int value". Test the custom message of the shared parser, and parse "benchmark --runs" through the NVX CLI parser to check that a non-integer value and zero keep their existing diagnostics. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Pedro Henrique Penna (ppenna)
force-pushed
the
code-reusability/copilot/code-reusability-positive-int-d9e5991dd2d58ebc
branch
from
October 4, 2026 18:03
a7a5070 to
7667f7a
Compare
Pedro Henrique Penna (ppenna)
deleted the
code-reusability/copilot/code-reusability-positive-int-d9e5991dd2d58ebc
branch
October 4, 2026 18:56
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.
Summary
This
[code-reusability]change reuses the existingcommon.positive_inthelper at the benchmark parser's second equivalent call site.scripts/nvx_tools/common.py::positive_intis already used by the performance, microVM, adversarial, and CLI parsing code.scripts/nvx_tools/benchmark.pypreviously reimplemented the sameint()conversion and positive-value check inline.must be at least 1diagnostic while all existing callers keep their default behavior. Benchmark'spositive_intstays a named wrapper rather than afunctools.partial, so argparse still reports a non-integer value asinvalid positive_int valueinstead of the partial's repr.PositiveIntTests.test_accepts_custom_error_messagecovers the shared helper's custom message, andCliTests.test_benchmark_positive_integer_diagnostics_are_stableparsesbenchmark --runsto check that non-integer and zero values keep their existing diagnostics.The patch changes
scripts/nvx_tools/common.py,scripts/nvx_tools/benchmark.py, andscripts/test_nvx_tools.py: 22 additions and 6 deletions (28 added-plus-deleted lines). It adds no dependency and makes no public API/CLI/ABI, gitlink, OpenVMM, workflow, or protected-file change.Validation
Re-run on Windows with Python 3.14 after rebasing onto
073de2724and addressing review feedback:python -m unittest scripts.test_nvx_tools.CliTests.test_benchmark_positive_integer_diagnostics_are_stable scripts.test_nvx_tools.PositiveIntTests -v— passed (5 tests); the CLI test fails whenbenchmark.positive_intis afunctools.partialpython -m unittest scripts/test_performance.py scripts/test_nvx_tools.py scripts/test_microvm_tests.py scripts/test_development_release.py scripts/test_egress_policy.py scripts/test_control_session.py scripts/test_managed_agent.py— passed (528 tests, 28 skipped)python scripts/test_adversarial.py— passed (70 tests, 6 skipped)python scripts/nvx.py benchmark --help— passedpython -m compileall -q scripts— passedpython -m ruff check scripts .github/specula— passedpython -m ruff format --check scripts .github/specula— passedpython -m pyright --pythonplatform Linux— passedpython -m pyright --pythonplatform Windows— passedgit diff --check— passed