feat: Annotated based argparse, and auto completer inference#1614
feat: Annotated based argparse, and auto completer inference#1614KelvinChung2000 wants to merge 11 commits intopython-cmd2:mainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1614 +/- ##
==========================================
+ Coverage 99.57% 99.63% +0.05%
==========================================
Files 21 22 +1
Lines 4721 5150 +429
==========================================
+ Hits 4701 5131 +430
+ Misses 20 19 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
I started working on this before I closed that PR. While this PR is still using LLMs, I have a much better understanding of what it is writing, since it is based on Python type processing, which I am much more familiar with than click. As mentioned, this code needs some more cleanup before it's ready for review; that's why this is a draft. However, I would like some feedback on the documentation and the example to make sure I haven't missed anything obvious. If you'd prefer to defer until it is fully ready, that's fine as well. |
|
I'm curious to see where this goes. I can't make any promises in advance, but it sounds like a potentially interesting feature. Though, I would prefer for all the tests to pass before I spend any time reviewing it. If the code isn't too complex so that it appears to integrate with the rest of If for some reason it doesn't immediately integrate well, there may be the possibility of creating a new open-source module that generates argparse argument parsers from type annotations. |
1834450 to
05c602e
Compare
tleonhardt
left a comment
There was a problem hiding this comment.
Overall I like the user experience of this better - there is one essentially consistent behavior throughout for help and completion all based on argparse.
I'm a little concerned about how much code this adds and if that might be a maintenance burden.
I left a few comments where I think there are a couple minor edge case bugs. I'll need to experiment some more with this once you address the comments here.
|
I am still working on some edge cases (particularly in groups and subcommands) and trying to simplify the code. If you still think it's too much, at least it is modular enough to be extracted and run as |
|
I'm not yet decided on whether its too much or not. I do really like the capability it provides and it is starting to shape up to something that feels like it could have a good user and developer experience. I just know we've been bitten a couple times in the past where we accepted features we shouldn't have and they turned into maintenance headaches. |
bcbaf18 to
73c241b
Compare
ecd5b98 to
8f0d70f
Compare
|
I'm not sure why, but the current version on GitHub appears to be the one before I added group support and did the code cleanup. Luckly vscode have cache the code changes... |
I'm not sure what happened there. Sorry about that. Let me know when you've pushed other changes and I can look again. If there are persistent issues, we can invite you as a Contributor so you can create a branch directly as long as you have 2-factor auth enabled in GitHub. |
|
@KelvinChung2000 This branch has conflicts with the |
ce60ddb to
a029288
Compare
| return | ||
| self._parser.print_help(file=file) | ||
|
|
||
| def _choices_to_items(self, arg_state: _ArgumentState) -> list[CompletionItem]: |
There was a problem hiding this comment.
The _resolve_enum function correctly adds the choices kwarg for Enum types, which means arg_state.action.choices will be populated (not None). Because this if block requires arg_state.action.choices is None, it will be skipped entirely for Enums defined via @with_annotated, falling back to the standard choices logic which does not populate display_meta.
You should check the action type regardless, and use choices to filter the returned members if it is populated.
Recommend something along these lines:
def _choices_to_items(self, arg_state: _ArgumentState) -> list[CompletionItem]:
"""Convert choices from action to list of CompletionItems."""
action_type = arg_state.action.type
if action_type is not None:
enum_class = None
if isinstance(action_type, type) and issubclass(action_type, enum.Enum):
enum_class = action_type
else:
enum_class = getattr(action_type, '_cmd2_enum_class', None)
if isinstance(enum_class, type) and issubclass(enum_class, enum.Enum):
if arg_state.action.choices is None:
return [CompletionItem(str(m.value), display_meta=m.name) for m in enum_class]
return [
CompletionItem(str(m.value), display_meta=m.name)
for m in enum_class
if m.value in arg_state.action.choices or m.name in arg_state.action.choices or m in arg_state.action.choices
]
if arg_state.action.choices is None and getattr(action_type, '__name__', None) == '_parse_bool':
return [CompletionItem(v) for v in ['true', 'false', 'yes', 'no', 'on', 'off', '1', '0']]
if arg_state.action.choices is None:
return []There was a problem hiding this comment.
Should I actually inline all of them to the annotated.py, such that the completer is only automatically inferred if you are using the annotation method? Depends on how one views this sort of automation: either too much magic or a very nice-to-have.
Another option would be removing the choice setting logic from the annotated.py and concentrating all of them here?
There was a problem hiding this comment.
I think the more we can keep the logic for this PR purely within annotated.py, the better.
cmd2/annotated.py
Outdated
| if metadata: | ||
| kwargs.update(metadata.to_kwargs()) | ||
| if metadata.nargs is not None: | ||
| kwargs['nargs'] = metadata.nargs |
There was a problem hiding this comment.
The action parameter of the Option class is completely dropped for non-boolean types.
The Option metadata explicitly accepts an action argument, but it is missing from _BaseArgMetadata._KWARGS_MAP and never injected back into the argparse kwargs during _resolve_type for non-boolean types. This means that a user providing Option(action='count') for an integer will have their action silently ignored by argparse.
Recommend adding a block after this, e.g.:
if metadata:
kwargs.update(metadata.to_kwargs())
if metadata.nargs is not None:
kwargs['nargs'] = metadata.nargs
if isinstance(metadata, Option) and getattr(metadata, 'action', None) is not None:
if 'action' not in kwargs:
kwargs['action'] = metadata.action
This is a full rework of #1612. Instead of wrapping Typer. We now extract the types and build the
argparseparser.I want to mark it as a draft for now, as some of the stuff will likely need a bit more cleanup. Please have a look at the documentation and example, and let me know if I missed anything obvious.