Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1635 +/- ##
==========================================
- Coverage 99.48% 99.48% -0.01%
==========================================
Files 21 21
Lines 4704 4699 -5
==========================================
- Hits 4680 4675 -5
Misses 24 24
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
…sive to better support order-independent stacking with other decorators.
| ChoicesProviderUnbound, | ||
| CmdOrSet, | ||
| CompleterUnbound, | ||
| CmdOrSetT, |
There was a problem hiding this comment.
Why change CmdOrSet to CmdOrSetT?
There was a problem hiding this comment.
CmdOrSet previously was a TypeVar.
It's now a TypeAlias and the TypeVar is called CmdOrSetT.
| if self._cmd_internal is None: | ||
| raise CommandSetRegistrationError('This CommandSet is not registered') | ||
| return self._cmd_internal | ||
| if (cmd := self._cmd_internal) is not None: |
There was a problem hiding this comment.
You can still use the walrus operator to do assignment in the conditional but check for the anomalous case in the if and return cmd afterwards. I think leaving it like that as a guard clause would be more readable.
There was a problem hiding this comment.
I simplified this code.
| CmdOrSet, | ||
| CompleterBound, | ||
| CompleterUnbound, | ||
| CmdOrSetT, |
There was a problem hiding this comment.
I am so confused. Why do we have both CmdOrSet and CmdOrSetT. Can we find a better name for one of them?
I intuitively get what CmdOrSet is.
I don't think I properly understand what CmdOrSetT is. I read CmdOrSetT as CmdOrSet Type, and CmdOrSet is literally the type alias for that. So the fact that we have something else that is CmdOrSetT confuses me. Perhaps we can give that later one a more self-describing name?
There was a problem hiding this comment.
This is about the difference between types (including TypeAlias) and TypeVar.
# A Cmd or CommandSet instance
CmdOrSet: TypeAlias = Union["Cmd", "CommandSet[Any]"]
# Tracks the specific subclass instance (either a Cmd or CommandSet)
CmdOrSetT = TypeVar("CmdOrSetT", bound=CmdOrSet)Adding T to a type communicates to other devs that it's a TypeVar bound to that type. It's still restricted to the same family of that type, but it adds the ability to track the specific one used.
This is why we also have a TypeVar for Cmd called CmdT. It allows us to make CommandSet a generic class which knows what type of Cmd it is loaded into.
Basically, TypeVars are important for preserving the type of something.
For instance:
# Using the TypeAlias: The IDE loses the specific class info
def get_my_app(app: CmdOrSet) -> CmdOrSet:
return app
# Using the TypeVar: The IDE remembers the specific class info
def get_my_app(app: CmdOrSetT) -> CmdOrSetT:
return app
# Result:
app = MyCustomApp()
my_app = get_my_app(app)
# With TypeAlias: 'my_app' is seen as just a 'Cmd'. You lose autocomplete for custom methods.
# With TypeVar: 'my_app' is recognized as 'MyCustomApp'. Autocomplete works.| args.cmd2_subcmd_handler(args) | ||
|
|
||
| @cmd2.as_subcommand_to('base', 'sub', sub_parser, help="the subcommand") | ||
| def sub_handler(self, args: argparse.Namespace) -> None: |
There was a problem hiding this comment.
Gemini complained about the type hint of None being incorrect in this example code in the docstring.
There was a problem hiding this comment.
An earlier commit mistakenly had the -> None on the as_subcommand_to decorator.
Is that what Gemini is referring to?
No description provided.