Skip to content

Fix with_annotated decorator so using ArgumentBlock works with groups#1718

Open
tleonhardt wants to merge 3 commits into
mainfrom
annotated_argumentblock
Open

Fix with_annotated decorator so using ArgumentBlock works with groups#1718
tleonhardt wants to merge 3 commits into
mainfrom
annotated_argumentblock

Conversation

@tleonhardt

@tleonhardt tleonhardt commented Jul 16, 2026

Copy link
Copy Markdown
Member

Cause

When an ArgumentBlock parameter is passed to @with_annotated(groups=...), the decorator resolves the argument spec based on the function's parameter names (e.g. block_argument). However, when parsing was eventually performed, block_argument had been completely expanded into its respective argument fields (e.g. A, B) which didn't possess the block_argument name directly, causing them to fall through as ungrouped arguments rather than being mapped to the user-supplied argument groups.

Fix

  1. Modified _ArgparseArgument to store the name of the block parameter it was expanded from (block_param_name)
  2. Updated _expand_dataclass_block to supply block_param_name when instantiating expanded arguments
  3. Updated build_parser_from_function to properly fall back to looking up targets via the block_param_name if an exact name match isn't found in target_for
  4. Updated _link_mutex_group_membership to map block_param_names to a list of its respective sub-fields, enabling mutually_exclusive_groups to also enforce constraints across all expanded block fields correctly
  5. Added tests (test_group_with_argument_block, test_mutex_group_with_argument_block) to explicitly test this functionality in tests/test_annotated.py

Closes #1714

@codecov

codecov Bot commented Jul 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.61%. Comparing base (8c036f7) to head (7cc0986).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1718      +/-   ##
==========================================
+ Coverage   99.59%   99.61%   +0.01%     
==========================================
  Files          23       23              
  Lines        5910     5915       +5     
==========================================
+ Hits         5886     5892       +6     
+ Misses         24       23       -1     
Flag Coverage Δ
unittests 99.61% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

@tleonhardt

Copy link
Copy Markdown
Member Author

@KelvinChung2000 I'd greatly appreciate your review as this is my first attempt at modifying the code in annotated.py.

@neoniobium Since you reported the issue and understand it, I'd also very much appreciate your review to verify that this fixes the underlying problem.

@KelvinChung2000 KelvinChung2000 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

While this is working for normal groups, as they are just normal basiclly an "or" collection of all the entry. But applying the same logic to me does not really works on mutually exclusive group and I would suggest rejecting this use case.

I would prefer a more explict API, something like

@dataclass
class Block(ArgumentBlock):
    A: ...
    B: ...


class App(Cmd):
    @with_annotated(groups=(Group("A", "B"), ...))
    def do_with_block(self, block_argument: Block) -> None:
        ...

Which is considered a typo today. But it will be clear in all cases. So the implementation for the above will be more like lifting/changing all the field checks from checking the parameter directly to, when it is an ArgumentBlock, checking whether it is part of the field instead. Since ArgumentBlock is a flat expansion in the parser, all the existing logic should be working already.

Comment thread cmd2/annotated.py
block_hint = hints.get(name)
if name == NS_ATTR_PARENT_ARGS:
inherited = _require_magic_block(name, block_hint, param, func.__qualname__)
_reject_field_shadowing_block_param(inherited, name, func.__qualname__)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
_reject_field_shadowing_block_param(inherited, name, func.__qualname__)
if any(name in spec.members for spec in mutually_exclusive_groups or ()):
raise TypeError("Cannot have BlockArgument for mutally exclusive groups as this is ambigious....")

Something similar, or can be extracted into a check function similar to _reject_field_shadowing_block_param for and have a more detailed error message

Comment thread cmd2/annotated.py

# An ArgumentBlock-typed parameter is an argument block: expand its fields in place (flat) instead of
# building a single argument for the container, so the fields keep their signature-order position.
if _is_argument_block(block_hint, param):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
if _is_argument_block(block_hint, param):
if any(name in spec.members for spec in mutually_exclusive_groups or ()):
raise TypeError("Cannot have BlockArgument for mutally exclusive groups as this is ambigious....")

Similar to above

@tleonhardt

Copy link
Copy Markdown
Member Author

@KelvinChung2000 If you have a better idea for how to handle this, I'm happy to cancel this PR and let you put up another one. Or if you want, feel free to take over this branch and modify as you see fit.

@KelvinChung2000
KelvinChung2000 force-pushed the annotated_argumentblock branch from 3c8a604 to 7cc0986 Compare July 16, 2026 23:46
@neoniobium

neoniobium commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

I checked out the PR and did some brief testing. The issue I raise is fully addressed with it. I happily do a technical review later. However I would prefer if it be decided weather this PR will be closed in favor a a different approach before I do that.

Edit: Ok, I see that @KelvinChung2000 commit. I assume this PR to be this to be the one that will address the issues.

@neoniobium neoniobium left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I have tested the PR in private project. Works as intended. The implementation look clean.

You might also want to update the example? However, do not have a strong opinion on weather this is too much for an example especially since the current example is also rather long.

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.

[with_annotated] Using ArgumentBlock does not works with groups

3 participants