Use subparser for argument parsing in simoc-sam.py#318
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors simoc-sam.py CLI argument parsing to use per-command subparsers generated from command function signatures, enabling --named=value arguments so decorators like @needs_root can re-invoke commands with kwargs.
Changes:
- Update
@needs_rootto re-run only the decorated command viasudo ... simoc-sam.py <cmd> --key=value .... - Replace the flat
cmd + *argsparser with per-command subparsers and a dispatcher that maps parsed args back onto function parameters. - Add a new pytest suite covering parser behavior,
main()dispatch behavior, and@needs_rootcommand construction.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
simoc-sam.py |
Introduces subparser-based parsing (create_parser) and a new main() dispatcher; updates needs_root to emit named CLI args for subprocess re-invocation. |
tests/test_simoc_sam_cli.py |
Adds tests for parsing, dispatch, and needs_root behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def test_parser_positional_args(): | ||
| """Test parsing with positional arguments only.""" | ||
| parser = simoc_sam_cli.create_parser() | ||
| args = parser.parse_args(['test-uid', 'wlan0', 'MyNetwork', 'password123']) | ||
|
|
||
| assert args.cmd == 'test-uid' | ||
| assert args._positional == ['wlan0', 'MyNetwork', 'password123'] | ||
|
|
There was a problem hiding this comment.
create_parser() builds subparsers only for commands present in COMMANDS, but these tests call parse_args() with the subcommand test-uid, which is not defined anywhere in simoc-sam.py (so argparse will raise SystemExit before assertions run). Consider using clean_commands and registering a small dummy command (e.g., test_uid) for these parser-only tests, or switch the test inputs to an actual existing command name.
| def create_help(cmds): | ||
| help = ['Full list of available commands:'] | ||
| for cmd, func in cmds.items(): | ||
| help.append(f'{cmd.replace("_", "-"):18} {func.__doc__}') | ||
| return '\n'.join(help) |
There was a problem hiding this comment.
create_help() appears to be unused after switching to subparsers (create_parser() no longer calls it, and there are no other references). Consider removing it to avoid dead code, or re-integrate it into the top-level ArgumentParser help output if you still want a consolidated command list.
| for param_name, param in params: | ||
| if param.kind == inspect.Parameter.VAR_POSITIONAL: | ||
| # Collect all remaining positional args | ||
| call_args.extend(positional[pos_idx:]) | ||
| break | ||
|
|
||
| # Check if provided via --flag (using SUPPRESS, so check hasattr) | ||
| if hasattr(args, param_name): | ||
| # Provided via --flag | ||
| named_value = getattr(args, param_name) | ||
| if param.default is inspect.Parameter.empty: | ||
| call_args.append(named_value) | ||
| else: | ||
| call_kwargs[param_name] = named_value | ||
| elif pos_idx < len(positional): | ||
| # Provided positionally | ||
| if param.default is inspect.Parameter.empty: | ||
| call_args.append(positional[pos_idx]) | ||
| else: | ||
| call_kwargs[param_name] = positional[pos_idx] | ||
| pos_idx += 1 | ||
| elif param.default is inspect.Parameter.empty: | ||
| # Required parameter not provided | ||
| parser.error(f'the following arguments are required: {param_name}') | ||
| # Otherwise: parameter has a default and wasn't provided, so don't pass it | ||
|
|
||
| cmd = args.cmd.replace('-', '_') | ||
| if cmd in COMMANDS: | ||
| result = COMMANDS[cmd](*args.args) | ||
| parser.exit(not result) | ||
| else: | ||
| cmds = ', '.join(cmd.replace('_', '-') for cmd in COMMANDS.keys()) | ||
| parser.error(f'Command not found. Available commands: {cmds}') | ||
| result = func(*call_args, **call_kwargs) | ||
| sys.exit(0 if result else 1) |
There was a problem hiding this comment.
main() doesn’t validate that all values in args._positional were consumed. For commands with no parameters (or fewer parameters than provided), extra positional arguments are silently ignored (e.g., teardown-wifi extra would still run). Consider adding a check after the loop to parser.error(...) when pos_idx < len(positional) and the signature doesn’t include *args, to avoid masking user mistakes.
This PR attempts to refactor the argument parsing to use custom subparsers for each
simoc-sam.pycommand. The goal is to allow passing named arguments -- a functionality needed by the@needs_rootrefactoring done in #317.For example, if we have the following command:
And we call it from another function as
test_cmd(10, kwarg3='test'),@needs_rootneeds a way to pass both10and'test'from the command line. Currently the arg parser only accepts positional args, so in order to passkwargs3we need to passkwarg1andkwarg2positionally, but doing so (i.e.python3 simoc-sam.py test-cmd 10 default None test) would pass'None'as a string.By adding support for named arguments,
@needs_rootwill be able to only passkwargs3(by callingpython simoc-sam.py test-cmd 10 --kwarg3=test).Since the implementation is rather tricky, I'm submitting this as a draft PR and will return to this later to review and complete the tests and polish the implementation -- for both the argparsing and the refactoring of
@needs_root.