Repository navigation
Omit self from instance method completions - #714
suryakant-source wants to merge 2 commits into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request updates the command completion logic in fire/completion.py to omit the implicit self argument of instance methods when generating command completions, and adds corresponding unit tests in fire/completion_test.py. Feedback suggests catching a broader Exception instead of just AttributeError in _IsUnboundMethod to improve robustness against mock objects or custom descriptors.
| try: | ||
| attr = inspect.getattr_static(owner, name) | ||
| except AttributeError: | ||
| return False |
There was a problem hiding this comment.
To make _IsUnboundMethod more robust against unexpected exceptions (e.g., when dealing with mock objects, custom descriptors, or environments with restricted/modified inspect modules), consider catching Exception instead of just AttributeError.
| try: | |
| attr = inspect.getattr_static(owner, name) | |
| except AttributeError: | |
| return False | |
| try: | |
| attr = inspect.getattr_static(owner, name) | |
| except Exception: | |
| return False |
|
@googlebot I signed it! |
1 similar comment
|
@googlebot I signed it! |
Fixes #382
Problem
For a class such as
python example.py -- --completionoffers--selfas a flag (opts="--arg1 --self ..."). Accessing an instance method on the class gives a plain function, and its first argument (self) was treated like any other flag.Change
_Commandsnow drops the first argument's flag for plain instance methods found on a class. Static methods and class methods are unaffected (checked withinspect.getattr_static). If a method has no positional args, nothing is dropped.Testing
testInstanceMethodCompletionsOmitSelfcovers instance, static and class methods. It fails without the change (('bar', '--self')is generated) and passes with it.I used an AI coding assistant while preparing this change, and I reviewed the diff and test results myself.