Skip to content

[core] Add argument parents#25

Merged
forfudan merged 2 commits intomainfrom
update
Mar 12, 2026
Merged

[core] Add argument parents#25
forfudan merged 2 commits intomainfrom
update

Conversation

@forfudan
Copy link
Owner

@forfudan forfudan commented Mar 12, 2026

This PR adds an argparse-style “parents” capability to ArgMojo so commands can reuse a shared set of argument definitions and constraint groups, with accompanying documentation and project-metadata updates.

Changes:

  • Add Command.add_parent(parent) to copy arguments + constraint groups from another Command.
  • Document the feature in the user manual and cross-library reference table; update planning/changelog.
  • Extend the pixi run test task to include a new parents test file; update .gitignore.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds an argparse-style “parents” capability to ArgMojo so commands can reuse a shared set of argument definitions and constraint groups, with accompanying documentation and project-metadata updates.

Changes:

  • Add Command.add_parent(parent) to copy arguments + constraint groups from another Command.
  • Document the feature in the user manual and cross-library reference table; update planning/changelog.
  • Extend the pixi run test task to include a new parents test file; update .gitignore.

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
src/argmojo/command.mojo Introduces add_parent() implementation and its docstring.
pixi.toml Adds tests/test_parents.mojo to the test task.
docs/user_manual.md Adds a new “Argument Parents …” section + TOC + method reference row.
docs/changelog.md Adds unreleased changelog entry for argument parents and mentions new tests.
docs/argmojo_overall_planning.md Marks feature as done and lists test_parents.mojo in repo structure.
README.md Small wording tweak about pre-1.0 stability.
.gitignore Adds ignore patterns for local test*.mojo / test*.py.

💡 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.

$ ./login --user alice --token secret --region eu
```

## Argument Parents and inheritance
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

Section title uses inconsistent capitalization (“Argument Parents and inheritance”) compared to the rest of the manual’s Title Case headings. Consider renaming to “Argument Parents and Inheritance” (and updating the TOC entry/anchor accordingly) for consistency.

Suggested change
## Argument Parents and inheritance
## Argument Parents and Inheritance

Copilot uses AI. Check for mistakes.
.choice["json"]()
.choice["yaml"]()
)
shared.mutually_exclusive(["json_output", "yaml_output"])
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

add_parent() docstring example calls shared.mutually_exclusive(["json_output", "yaml_output"]), but neither argument is defined in the example. Since group constraints are validated at registration time, this example would raise immediately; please update it to reference registered argument names or remove the line.

Suggested change
shared.mutually_exclusive(["json_output", "yaml_output"])

Copilot uses AI. Check for mistakes.
self.subcommands.append(h^)
self.subcommands.append(sub^)

fn add_parent(mut self, parent: Command) raises:
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

add_parent takes parent: Command by value. Since Command.__copyinit__ performs a deep copy, passing an existing parent variable will allocate/copy its args/groups/subcommands even though this method only needs read access. Consider taking parent: borrowed Command (or otherwise avoiding a full copy) to prevent unnecessary work for large parent definitions.

Suggested change
fn add_parent(mut self, parent: Command) raises:
fn add_parent(mut self, parent: borrowed Command) raises:

Copilot uses AI. Check for mistakes.
Comment on lines +631 to +645
for i in range(len(parent.args)):
self.add_argument(parent.args[i].copy())
for i in range(len(parent._exclusive_groups)):
self._exclusive_groups.append(parent._exclusive_groups[i].copy())
for i in range(len(parent._required_groups)):
self._required_groups.append(parent._required_groups[i].copy())
for i in range(len(parent._one_required_groups)):
self._one_required_groups.append(
parent._one_required_groups[i].copy()
)
for i in range(len(parent._conditional_reqs)):
self._conditional_reqs.append(parent._conditional_reqs[i].copy())
for i in range(len(parent._implications)):
self._implications.append(parent._implications[i].copy())

Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

add_parent() mutates self incrementally: it appends inherited args one-by-one and then appends inherited constraint groups. If an inherited argument fails add_argument() validation (e.g., name/short/long conflict with an existing child arg), the method will raise after partially modifying self, leaving a subset of parent args added and no constraints copied yet. Consider preflight validation (detect conflicts up front) or staging changes in temporaries and only committing them once all inherited args/groups are known to succeed.

Suggested change
for i in range(len(parent.args)):
self.add_argument(parent.args[i].copy())
for i in range(len(parent._exclusive_groups)):
self._exclusive_groups.append(parent._exclusive_groups[i].copy())
for i in range(len(parent._required_groups)):
self._required_groups.append(parent._required_groups[i].copy())
for i in range(len(parent._one_required_groups)):
self._one_required_groups.append(
parent._one_required_groups[i].copy()
)
for i in range(len(parent._conditional_reqs)):
self._conditional_reqs.append(parent._conditional_reqs[i].copy())
for i in range(len(parent._implications)):
self._implications.append(parent._implications[i].copy())
# Perform inheritance on a temporary copy of `self` so that
# validation failures (e.g. in `add_argument`) do not leave this
# command in a partially updated state.
var tmp = self.copy()
# First, try to add all inherited arguments to the temporary command.
for i in range(len(parent.args)):
tmp.add_argument(parent.args[i].copy())
# If argument addition succeeded, copy over all constraint groups.
for i in range(len(parent._exclusive_groups)):
tmp._exclusive_groups.append(parent._exclusive_groups[i].copy())
for i in range(len(parent._required_groups)):
tmp._required_groups.append(parent._required_groups[i].copy())
for i in range(len(parent._one_required_groups)):
tmp._one_required_groups.append(
parent._one_required_groups[i].copy()
)
for i in range(len(parent._conditional_reqs)):
tmp._conditional_reqs.append(
parent._conditional_reqs[i].copy()
)
for i in range(len(parent._implications)):
tmp._implications.append(parent._implications[i].copy())
# All operations succeeded; commit the staged changes back to `self`.
self.args = tmp.args
self._exclusive_groups = tmp._exclusive_groups
self._required_groups = tmp._required_groups
self._one_required_groups = tmp._one_required_groups
self._conditional_reqs = tmp._conditional_reqs
self._implications = tmp._implications

Copilot uses AI. Check for mistakes.
Comment on lines +45 to +46
&& mojo run -I src -D ASSERT=all tests/test_prompt.mojo < /dev/null \
&& mojo run -I src -D ASSERT=all tests/test_parents.mojo"""
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

The test task now runs tests/test_parents.mojo, but that file is not present in the repository. As written, pixi run test will fail; either add/commit the new test file or remove this invocation.

Suggested change
&& mojo run -I src -D ASSERT=all tests/test_prompt.mojo < /dev/null \
&& mojo run -I src -D ASSERT=all tests/test_parents.mojo"""
&& mojo run -I src -D ASSERT=all tests/test_prompt.mojo < /dev/null"""

Copilot uses AI. Check for mistakes.
- Add Developer Validation section to user manual documenting the two-layer validation model (compile-time `StringLiteral` + runtime registration-time `raises`) with recommended workflow (PR #22).
- Add `pixi run debug` task that runs all examples under `-D ASSERT=all` with `--help` to exercise registration-time validation in CI (PR #22).
- Add `tests/test_prompt.mojo` with tests covering interactive prompting builder methods, optional/required prompt arguments, prompting skipped when values are provided, choices and defaults integration, field propagation through copy, and combined features (PR #23).
- Add `tests/test_parents.mojo` with 20 tests covering argument parents: basic flag/value/positional/default inheritance, short flags, multiple parents, child-own args coexistence, group constraint inheritance (mutually exclusive, required together, one-required, conditional, implications), parent shared across children, count/append/range argument inheritance, empty parent, parent immutability, and parent with subcommands (PR #25).
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

The changelog claims tests/test_parents.mojo was added, but the file is not present in the repo. Please either include/commit the new test file or adjust this entry so the changelog matches what was actually added.

Suggested change
- Add `tests/test_parents.mojo` with 20 tests covering argument parents: basic flag/value/positional/default inheritance, short flags, multiple parents, child-own args coexistence, group constraint inheritance (mutually exclusive, required together, one-required, conditional, implications), parent shared across children, count/append/range argument inheritance, empty parent, parent immutability, and parent with subcommands (PR #25).
- Add tests covering argument parents: 20 tests covering basic flag/value/positional/default inheritance, short flags, multiple parents, child-own args coexistence, group constraint inheritance (mutually exclusive, required together, one-required, conditional, implications), parent shared across children, count/append/range argument inheritance, empty parent, parent immutability, and parent with subcommands (PR #25).

Copilot uses AI. Check for mistakes.
Comment on lines +170 to +171
├── test_prompt.mojo # interactive prompting tests
└── test_parents.mojo # argument parents (shared definitions) tests
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

This repository-structure section lists tests/test_parents.mojo, but that file does not exist in the repo. Please add/commit the test file or update the documented tree so it stays accurate.

Suggested change
├── test_prompt.mojo # interactive prompting tests
└── test_parents.mojo # argument parents (shared definitions) tests
└── test_prompt.mojo # interactive prompting tests

Copilot uses AI. Check for mistakes.
- Fix title capitalization: 'Argument Parents and inheritance' →
  'Argument Parents and Inheritance' (TOC + heading)
- Remove incorrect docstring example (mutually_exclusive with undefined
  argument names)
- Make add_parent() atomic: rollback partially-added args on failure
  so self is never left in a half-inherited state
@forfudan forfudan merged commit 10da0cf into main Mar 12, 2026
2 checks passed
@forfudan forfudan deleted the update branch March 12, 2026 17:23
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.

2 participants