Conversation
There was a problem hiding this comment.
Pull request overview
Adds an interactive prompting capability to ArgMojo so arguments marked for prompting can be requested from the user when missing at parse time.
Changes:
- Add
Argument.prompt()/Argument.prompt["custom text"]()builder methods and store prompt metadata onArgument. - Prompt for missing prompt-enabled arguments during
Command.parse_arguments()before defaults/validation. - Add a new test suite plus docs and demo updates describing the feature.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
tests/test_prompt.mojo |
New tests covering the prompt builder API and “prompt skipped when provided” behavior. |
src/argmojo/command.mojo |
Hooks prompting into parse_arguments() and implements _prompt_missing_args(). |
src/argmojo/argument.mojo |
Adds _prompt / _prompt_text fields and the .prompt() builder overloads. |
pixi.toml |
Runs the new prompt test with stdin redirected to avoid blocking. |
examples/demo.mojo |
Adds a login subcommand demonstrating prompting and updates the feature list. |
docs/user_manual.md |
Documents interactive prompting and updates builder method references/tables. |
docs/changelog.md |
Adds an unreleased changelog entry for interactive prompting. |
docs/argmojo_overall_planning.md |
Marks interactive prompting as done and lists the new test file/feature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| Called after the parsing loop but before ``_apply_defaults()``, so | ||
| that user-provided prompt responses take priority and defaults | ||
| fill in only remaining gaps. For flag arguments, the prompt | ||
| accepts ``y``/``n`` (case-insensitive). For arguments with | ||
| choices, valid options are shown. For arguments with a default, | ||
| the default is displayed in parentheses and used when the user | ||
| enters nothing. | ||
|
|
||
| Prompting is skipped entirely when no arguments have | ||
| ``.prompt()`` enabled. When stdin is not a terminal (e.g., |
There was a problem hiding this comment.
The _prompt_missing_args() docstring says it’s called “before _apply_defaults()”, but parse_arguments() currently calls it after defaults. Either update the docstring or (preferably) reorder the call sites to match, otherwise the documented default/empty-input behavior isn’t achievable.
| Called after the parsing loop but before ``_apply_defaults()``, so | |
| that user-provided prompt responses take priority and defaults | |
| fill in only remaining gaps. For flag arguments, the prompt | |
| accepts ``y``/``n`` (case-insensitive). For arguments with | |
| choices, valid options are shown. For arguments with a default, | |
| the default is displayed in parentheses and used when the user | |
| enters nothing. | |
| Prompting is skipped entirely when no arguments have | |
| ``.prompt()`` enabled. When stdin is not a terminal (e.g., | |
| This is called after the main parsing loop so that user-provided | |
| prompt responses can take priority over any existing values in | |
| the parse result. For flag arguments, the prompt accepts | |
| ``y``/``n`` (case-insensitive). For arguments with choices, | |
| valid options are shown. For arguments with a default, the | |
| default is displayed in parentheses and used when the user | |
| enters nothing. | |
| Prompting is skipped entirely when no arguments have | |
| ``.prompt()`` enabled. When stdin is not a terminal (e.g., |
| elif a._is_map: | ||
| self._store_map_value(a, value, result) | ||
| elif a._is_append or a._number_of_values > 1: | ||
| self._store_append_value(a, value, result) |
There was a problem hiding this comment.
The prompt path treats a._number_of_values (nargs) as an append-style single value by calling _store_append_value(a, value, result) once. That diverges from normal parsing, where nargs consumes exactly N separate tokens and stores N list entries. Consider prompting N times (or parsing/splitting a single line into N values and enforcing the count) so number_of_values[N]() behaves consistently under prompting.
| self._store_append_value(a, value, result) | |
| # For arguments that append or accept multiple values (nargs), | |
| # ensure we store one entry per value, matching normal parsing. | |
| if a._number_of_values > 1: | |
| let parts = value.split() | |
| if len(parts) != a._number_of_values: | |
| self._warn( | |
| "Invalid number of values for '" | |
| + a.name | |
| + "': expected " | |
| + String(a._number_of_values) | |
| + ", got " | |
| + String(len(parts)) | |
| ) | |
| # Do not store partial/invalid data; allow defaults | |
| # or other validation to handle this argument. | |
| continue | |
| for part in parts: | |
| self._store_append_value(a, part, result) | |
| else: | |
| self._store_append_value(a, value, result) |
| Since interactive prompting reads from stdin, these tests focus on: | ||
| 1. Builder method correctness (`.prompt()`, `.prompt["custom text"]()`) | ||
| 2. Prompting is SKIPPED when arguments are provided on the command line | ||
| 3. Prompt fields are set correctly on Argument instances | ||
| 4. Choices/defaults appear in the prompt (tested via field inspection) |
There was a problem hiding this comment.
The test suite here doesn’t exercise the “happy path” where input() returns a non-empty response (i.e. _prompt_missing_args() actually stores prompted values, validates choices, and interacts with defaults). Since prompting is the core feature being added, it’d be valuable to add at least one integration-style test that runs with stdin preloaded (e.g., redirect from a fixture file) and asserts the parsed results.
| # Apply defaults, propagate implications, prompt for remaining | ||
| # values, re-apply implications (in case prompts triggered new | ||
| # ones), then validate constraints. | ||
| self._apply_defaults(result) | ||
| self._apply_implications(result) | ||
| self._prompt_missing_args(result) |
There was a problem hiding this comment.
parse_arguments() applies defaults before calling _prompt_missing_args(). Because _apply_defaults() writes defaults into result and ParseResult.has() treats any stored value as “provided”, prompt-enabled args that also have .default[]() will never prompt (and users can’t press Enter to accept the default or override it interactively). Consider reordering to prompt before defaults (e.g. parse → apply implications → prompt → apply defaults → re-apply implications → validate) so prompting works as documented for defaulted args.
| # Apply defaults, propagate implications, prompt for remaining | |
| # values, re-apply implications (in case prompts triggered new | |
| # ones), then validate constraints. | |
| self._apply_defaults(result) | |
| self._apply_implications(result) | |
| self._prompt_missing_args(result) | |
| # Propagate implications from explicitly provided args, prompt for | |
| # remaining values, apply defaults for anything still missing, | |
| # re-apply implications (in case prompts/defaults triggered new | |
| # ones), then validate constraints. | |
| self._apply_implications(result) | |
| self._prompt_missing_args(result) | |
| self._apply_defaults(result) |
This PR adds an interactive prompting capability to ArgMojo so arguments marked for prompting can be requested from the user when missing at parse time.
Changes:
Argument.prompt()/Argument.prompt["custom text"]()builder methods and store prompt metadata onArgument.Command.parse_arguments()before defaults/validation.