Skip to content

BUG: Allow string/path lists for multi-value Python API arguments - #97

Open
axel-grc wants to merge 3 commits into
RTKConsortium:mainfrom
axel-grc:argparse
Open

BUG: Allow string/path lists for multi-value Python API arguments#97
axel-grc wants to merge 3 commits into
RTKConsortium:mainfrom
axel-grc:argparse

Conversation

@axel-grc

@axel-grc axel-grc commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Multi-value (nargs="+") options were only comma-split for non-str types
because the neutralization gate skipped str-typed arguments. Passing a
list of paths through the Python API, e.g.
pct.pctcheckimagequality(reference=["a.nrrd","b.nrrd"]), serialized as
a single comma token but was never split, yielding one bogus filename
"a.nrrd,b.nrrd".

Neutralize the type of every nargs="+" option, including str/path lists,
then split comma tokens and re-cast each piece (str cast is a no-op).

Multi-value (nargs="+") options were only comma-split for non-str types
because the neutralization gate skipped str-typed arguments. Passing a
list of paths through the Python API, e.g.
pct.pctcheckimagequality(reference=["a.nrrd","b.nrrd"]), serialized as
a single comma token but was never split, yielding one bogus filename
"a.nrrd,b.nrrd".

Neutralize the type of every nargs="+" option, including str/path lists,
then split comma tokens and re-cast each piece (str cast is a no-op).

@SimonRit SimonRit left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm mostly unable to read this code. It would be good to add some documentation to required_dests and is there any way of testing this?

@acoussat

acoussat commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

I created a small test: acoussat@4bbce8d. The test does not pass with the previous version of the code but works using this branch. Should I add this test to this PR, or perhaps even push it to RTK?

@SimonRit

SimonRit commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

I created a small test: acoussat@4bbce8d. The test does not pass with the previous version of the code but works using this branch. Should I add this test to this PR, or perhaps even push it to RTK?

Here is good I believe. Thanks!

@axel-grc

axel-grc commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks @acoussat !
I think we could add one more test with the terminal-like command line in 1 string :
args = parser.parse_args( "--string-single a --number-single 1 --string-many a,b,c --number-many 1,2,3" )

@acoussat

acoussat commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

ArgumentParser.parse_args is not designed to parse a string directly, so we cannot test it with one single string.

https://docs.python.org/3/library/argparse.html#option-value-syntax
https://stackoverflow.com/questions/67368666/parsing-a-string-using-argparse

axel-grc and others added 2 commits September 4, 2026 09:30
- Collapse the numeric-only neutralization loop and the two split/cast
  branches in parse_args into a single unified pass for every nargs="+"
  option, and restore original types in a finally block so the parser
  stays reusable across calls.
- Extract required_dests() shared by build_signature() and
  build_usage_examples().
- Simplify option-flag selection in parse_kwargs().
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