feat: Annotated based argparse, and auto completer inference#1614
feat: Annotated based argparse, and auto completer inference#1614KelvinChung2000 wants to merge 7 commits intopython-cmd2:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1614 +/- ##
==========================================
- Coverage 99.53% 99.38% -0.16%
==========================================
Files 21 22 +1
Lines 4758 5031 +273
==========================================
+ Hits 4736 5000 +264
- Misses 22 31 +9
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
| hints = {} | ||
|
|
||
| for name, param in sig.parameters.items(): | ||
| if name == 'self': |
There was a problem hiding this comment.
If a user follows a different naming convention for the method's first parameter than self (e.g., this or cls), it will be incorrectly treated as a command-line argument. It is safer to skip the first parameter by position, as cmd2 commands are intended to be methods.
Perhaps something like this?
parameters = list(sig.parameters.values())
if parameters:
parameters.pop(0)
for param in parameters:
name = param.name|
|
||
| if isinstance(action_type, type) and issubclass(action_type, enum.Enum): | ||
| return [CompletionItem(str(m.value), display_meta=m.name) for m in action_type] | ||
|
|
There was a problem hiding this comment.
If I understand correctly, positional boolean arguments lack tab completion. While positional boolean args are pretty uncommon, positional arguments using _parse_bool (which is assigned by the decorator) don't have explicit choices, so the completer will only show a generic hint. Providing standard boolean strings as completion items would improve the UX.
Recommend adding something like this here:
# Check for boolean converter assigned by @with_annotated
if action_type.__name__ == '_parse_bool':
return [CompletionItem(v) for v in ['true', 'false', 'yes', 'no', 'on', 'off', '1', '0']]| command_name = fn.__name__[len(constants.COMMAND_FUNC_PREFIX) :] | ||
|
|
||
| @functools.wraps(fn) | ||
| def cmd_wrapper(*args: Any, **_kwargs: Any) -> bool | None: |
There was a problem hiding this comment.
The cmd_wrapper captures **_kwargs but they are never used or passed to the wrapped function fn. If with_annotated is used in a context where other decorators pass keyword arguments, they will be lost.
Recommend something like:
def cmd_wrapper(*args: Any, **kwargs: Any) -> bool | None:
...
func_kwargs.update(kwargs)
result: bool | None = fn(owner, **func_kwargs)
return result|
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. |
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.