feat(ext.argparse): add read-only _command_meta accessor (#670)#787
Open
derks wants to merge 6 commits into
Open
feat(ext.argparse): add read-only _command_meta accessor (#670)#787derks wants to merge 6 commits into
derks wants to merge 6 commits into
Conversation
- Add ArgparseController._command_meta read-only @Property returning the currently-dispatched command's CommandMeta, or None outside a dispatched command (no __dispatch__ on pargs); never raises (D-01..D-04). - Resolve meta via class-level getattr(self.__class__, func_name) to match the shared CommandMeta whose .controller is mutated in _collect_commands (D-07 sharing caveat). - Leave func() dispatch signature unchanged (additive-only, 3.0.x BC); record Cement 4 func-signature-injection deferral as a comment (D-06/D-07). - Resolves #670 ergonomic call site self._command_meta.parser_options['help'].
- Add command_meta_cmd fixture on Base reading self._command_meta inside its body and returning label + parser_options['help'] (the #670 call site). - test_command_meta_dispatched asserts the @expose(help=...) string and label round-trip (D-02/D-04). - test_command_meta_none_outside_dispatch asserts None on the no-sub-command path where pargs has no __dispatch__ (D-03), via Base._default body read. - Update test_base_default to match _default's new meta-aware return string. - 100% coverage on cement/ext/ext_argparse.py with zero misses.
- Add [ext.argparse] Features entry under the active 3.0.15 - DEVELOPMENT section documenting the read-only self._command_meta accessor. - Note it is additive (func() dispatch signature unchanged) and returns None outside a dispatched command. - Trailing [Issue #670] sub-bullet per the established entry shape.
Code review WR-01: _command_meta also runs on controllers that do not own the dispatched command (e.g. via _post_argument_parsing). Guard on the __cement_meta__ marker instead of bare attribute existence so a same-named, non-command attribute yields None rather than raising AttributeError. Collapses the guard and removes the pragma: nocover defensive line; adds a non-owning-controller regression test. Still additive and read-only -- no 3.0.x public-API change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… consent Add a Branching Policy section to CLAUDE.md codifying that no work -- implementation, tests, docs, or planning artifacts -- may be committed directly to main without explicit consent, and that a branch must be created immediately when phase/task work begins (not deferred to execution). Notes that GSD branching_strategy defaults to 'none' and must not be relied on to branch automatically. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdded ChangesArgparse command metadata
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cement/ext/ext_argparse.py`:
- Around line 382-393: The current command lookup in the `CommandMeta`-returning
property only uses `func_name`, so a non-owning controller with the same method
name can incorrectly return its own `__cement_meta__`. Update the lookup to also
verify the dispatched controller label from `self.app.pargs.__dispatch__`
matches the owning controller before reading `member` and `meta`, and return
`None` when the label does not match. Keep the fix localized to the property
that resolves `CommandMeta` and preserve the existing never-raises behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8527463a-230d-4d5b-9466-56c45070fafb
📒 Files selected for processing (4)
CHANGELOG.mdCLAUDE.mdcement/ext/ext_argparse.pytests/ext/test_ext_argparse.py
Comment on lines
+382
to
+393
| if not hasattr(self.app.pargs, '__dispatch__'): | ||
| return None | ||
| func_name = self.app.pargs.__dispatch__.split('.')[1] | ||
| # Resolve the class-level command function and read its CommandMeta. | ||
| # Guarding on the __cement_meta__ marker (rather than mere attribute | ||
| # existence) keeps the never-raises contract: this property can also | ||
| # run on a non-owning controller (e.g. via _post_argument_parsing), | ||
| # whose class may carry a same-named, non-command attribute -- that | ||
| # yields None here instead of an AttributeError (WR-01). | ||
| member = getattr(self.__class__, func_name, None) | ||
| meta: CommandMeta | None = getattr(member, '__cement_meta__', None) | ||
| return meta |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Gate the lookup on the dispatched controller label.
Right now this only keys off func_name, so a non-owning controller that happens to expose the same method name will return its own __cement_meta__ instead of None. That breaks the new “current command” contract even though the non-command collision test passes.
Suggested fix
- if not hasattr(self.app.pargs, '__dispatch__'):
+ dispatch = getattr(self.app.pargs, '__dispatch__', None)
+ if dispatch is None:
return None
- func_name = self.app.pargs.__dispatch__.split('.')[1]
+ contr_label, _, func_name = dispatch.partition('.')
+ if not func_name or contr_label != self._meta.label:
+ return None
# Resolve the class-level command function and read its CommandMeta.
# Guarding on the __cement_meta__ marker (rather than mere attribute
# existence) keeps the never-raises contract: this property can also
# run on a non-owning controller (e.g. via _post_argument_parsing),
# whose class may carry a same-named, non-command attribute -- that
# yields None here instead of an AttributeError (WR-01).
member = getattr(self.__class__, func_name, None)
meta: CommandMeta | None = getattr(member, '__cement_meta__', None)
return meta📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if not hasattr(self.app.pargs, '__dispatch__'): | |
| return None | |
| func_name = self.app.pargs.__dispatch__.split('.')[1] | |
| # Resolve the class-level command function and read its CommandMeta. | |
| # Guarding on the __cement_meta__ marker (rather than mere attribute | |
| # existence) keeps the never-raises contract: this property can also | |
| # run on a non-owning controller (e.g. via _post_argument_parsing), | |
| # whose class may carry a same-named, non-command attribute -- that | |
| # yields None here instead of an AttributeError (WR-01). | |
| member = getattr(self.__class__, func_name, None) | |
| meta: CommandMeta | None = getattr(member, '__cement_meta__', None) | |
| return meta | |
| dispatch = getattr(self.app.pargs, '__dispatch__', None) | |
| if dispatch is None: | |
| return None | |
| contr_label, _, func_name = dispatch.partition('.') | |
| if not func_name or contr_label != self._meta.label: | |
| return None | |
| # Resolve the class-level command function and read its CommandMeta. | |
| # Guarding on the __cement_meta__ marker (rather than mere attribute | |
| # existence) keeps the never-raises contract: this property can also | |
| # run on a non-owning controller (e.g. via _post_argument_parsing), | |
| # whose class may carry a same-named, non-command attribute -- that | |
| # yields None here instead of an AttributeError (WR-01). | |
| member = getattr(self.__class__, func_name, None) | |
| meta: CommandMeta | None = getattr(member, '__cement_meta__', None) | |
| return meta |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cement/ext/ext_argparse.py` around lines 382 - 393, The current command
lookup in the `CommandMeta`-returning property only uses `func_name`, so a
non-owning controller with the same method name can incorrectly return its own
`__cement_meta__`. Update the lookup to also verify the dispatched controller
label from `self.app.pargs.__dispatch__` matches the owning controller before
reading `member` and `meta`, and return `None` when the label does not match.
Keep the fix localized to the property that resolves `CommandMeta` and preserve
the existing never-raises behavior.
Member
Author
|
@TomFreudenberg does this meet your needs? From within a controller method/command: @ex(help='my command does the thing')
def my_command(self):
cmd_meta = self._command_meta # CommandMeta | None
my_help = cmd_meta.parser_options['help'] # -> 'my command does the thing' |
Contributor
|
Hi @derks it is exactly handled as I do currently within https://github.com/tokeo/tokeo/blob/master/tokeo/core/utils/controllers.py maybe you also add the access to the default subcommand mtea? def defaultmeta(controller):
"""
Access the \\_\\_cement_meta\\_\\_ information of default subcommand within the
command.
### Args
- **controller** (Controller): The controller instance
### Returns
- **\\_\\_cement_meta\\_\\_** (Meta): object ref to Meta information of the
_default_ subcommand
"""
return getattr(controller, controller._meta.default_func).__cement_meta__ |
Companion to _command_meta for the default sub-command path. argparse has no native default sub-command, so Cement reaches the default via Meta.default_func rather than the __dispatch__ token -- which is why _command_meta is None while the default function runs. _default_command_meta resolves the default function's CommandMeta directly so a default command can read its own @ex/@expose meta. Returns None when default_func is None or points to a non-exposed function (e.g. the framework's stock _default print_help), mirroring the never-raises contract. Additive and read-only -- no 3.0.x public-API change. Addresses @TomFreudenberg's PR #787 request. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Member
Author
|
@TomFreudenberg added The default path can't reuse One thing to flag on your Usage: @ex(help='my command does the thing')
def _default(self):
cmd_meta = self._default_command_meta # CommandMeta | None
my_help = cmd_meta.parser_options['help'] # -> 'my command does the thing'Pushing it up to this PR now. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a single additive, read-only
_command_metaproperty toArgparseController, resolving #670.An exposed
@ex/@exposecommand can now read its ownCommandMetafrominside its body via
self._command_meta— e.g.self._command_meta.parser_options['help']— instead of the brittlegetattr(self, self.app.pargs.__dispatch__.split('.')[1]).__cement_meta__dance (which broke in the wild once
CommandMetabecame a dataclass).The change is purely additive and backward-compatible on the 3.0.x track:
the released
func()dispatch signature is untouched. The issue's literalfunc(func_name, func_meta)injection proposal is intentionally not adoptedhere — it would change the released dispatch signature and is deferred to
Cement 4 (recorded as a
# FIXME:near the accessor and at_dispatch).Changes
Accessor (feat)
ArgparseController._command_meta@propertyreturningCommandMeta | None. Derives the running command fromself.app.pargs.__dispatch__, resolves the class-levelCommandMetavia thefunction's
__cement_meta__marker, and returnsNoneoutside a dispatchedcommand. Never raises.
Robustness hardening (fix — from code review)
__cement_meta__marker(
getattr(member, '__cement_meta__', None)) rather than bare attributeexistence. This makes the documented "never raises" contract provably hold
even when the property is read from a non-owning controller (e.g. via
_post_argument_parsing) whose class happens to carry a same-named,non-command attribute — that path now returns
Noneinstead of raisingAttributeError. The change also collapses both guard branches and removes a# pragma: nocoverdefensive line.Tests
test_command_meta_dispatched— command reads its own@expose(help=...).test_command_meta_none_outside_dispatch— no-dispatch path returnsNone.test_command_meta_non_owning_controller_returns_none— non-owningcontroller returns
None(never raises).Docs
CHANGELOG.md:[ext.argparse]Features entry referencing Push function name and meta of sub-command into function #670.CLAUDE.md: adds a Branching Policy section — never commit tomainwithout explicit consent (planning artifacts included), and branch
immediately when phase/task work begins rather than relying on GSD's
branching_strategyconfig.Acceptance status
_command_metaproperty onArgparseControllerCommandMetafor the running command frompargs.__dispatch__None(never raises) when no command is dispatchedCommandMetaas-is (no wrapper/shortcuts)func()dispatch signature unchanged (no signature injection)_dispatchVerified 5/5 must-have truths against the codebase (full report archived
post-merge with the phase planning artifacts).
Files touched
cement/ext/ext_argparse.py_command_metaproperty (additive, read-only)tests/ext/test_ext_argparse.pyCHANGELOG.md[ext.argparse]Features entry (#670)CLAUDE.mdTest plan
make comply— ruff clean + mypy clean (51 source files)make test— full suite 375 passed, 100.00% coverage(Redis + memcached up);
ext_argparse.py318/318 stmts, 0 missreturn func()at dispatch is unchanged;every existing
def cmd(self)still receives zero extra positional argsSummary by CodeRabbit