Repository navigation
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1806 +/- ##
=======================================
Coverage 99.75% 99.75%
=======================================
Files 23 23
Lines 6593 6596 +3
=======================================
+ Hits 6577 6580 +3
Misses 16 16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
There was a problem hiding this comment.
@Qrytics
Thanks for the PR Mario! Wrapping the error and naming the offending callable is the right direction, and much better than leaking a raw NameError. A few changes requested inline. One more that has no line to attach to:
- CHANGELOG: please add a bug-fix entry to
CHANGELOG.mdunder a new in-progress version heading (4.4.1 is already released).
| """ | ||
| try: | ||
| type_hints = inspect.get_annotations(func_or_method, eval_str=True) # Get dictionary of type hints | ||
| except TypeError as exc: |
There was a problem hiding this comment.
This existing handler also swallows a TypeError raised while evaluating an annotation. For example, def b(x: "int[str]") reports "Argument passed to get_types should be a function or method", which is wrong.
Action: distinguish the two cases, e.g. check callable(func_or_method) up front for the invalid-argument error, then treat any TypeError from get_annotations as a resolution failure.
| type_hints = inspect.get_annotations(func_or_method, eval_str=True) # Get dictionary of type hints | ||
| except TypeError as exc: | ||
| raise ValueError("Argument passed to get_types should be a function or method") from exc | ||
| except (NameError, SyntaxError) as exc: |
There was a problem hiding this comment.
AttributeError still leaks: def a(x: "typing.NoSuch") raises AttributeError: module 'typing' has no attribute 'NoSuch'.
Action: add AttributeError to this tuple. _resolve_func_hints in cmd2/annotated.py catches (NameError, AttributeError, TypeError) for the same failure.
| raise ValueError("Argument passed to get_types should be a function or method") from exc | ||
| except (NameError, SyntaxError) as exc: | ||
| name = getattr(func_or_method, "__name__", func_or_method) | ||
| raise ValueError(f"Unable to resolve type hints for {name}: {exc}") from exc |
There was a problem hiding this comment.
Consider raising TypeError here instead of ValueError. The only callers of get_types are the hook-registration validators in cmd2/cmd2.py, which raise TypeError for every other annotation problem, and _resolve_func_hints in cmd2/annotated.py already raises TypeError("Failed to resolve type hints for ...") for this exact failure.
Action: raise TypeError for unresolvable annotations and update the :raises docstring to match.
| assert param_value is int | ||
|
|
||
|
|
||
| def test_get_types_unresolvable_forward_reference() -> None: |
There was a problem hiding this comment.
Only the NameError path is tested.
Action: add cases (parametrizing this test would work well) for:
- invalid syntax, e.g.
"1 +" - a bad attribute, e.g.
"typing.NoSuch" - a
TypeErrorduring evaluation, e.g."int[str]"
Please also add one test that registers a hook with an unresolvable annotation (e.g. via register_postparsing_hook) and asserts the resulting error.
What
cmd2.utils.get_typesdocuments that it raisesValueErrorwhen its inputcan't be processed, but an unresolvable string annotation (e.g. a forward
reference under
from __future__ import annotations, or aTYPE_CHECKING-only import) leaks a raw
NameError(orSyntaxErrorfor invalidannotation strings) out of
inspect.get_annotations(..., eval_str=True).That surfaces as a confusing crash when registering hooks/commands instead
of the documented error type.
Repro
Fix
Catch
(NameError, SyntaxError)around theget_annotationscall andre-raise as
ValueErrornaming the offending callable, mirroring theexisting
TypeErrorhandling in the same function. No success pathchanges.
Tests
Added
test_get_types_unresolvable_forward_referenceintests/test_utils.py(fails before withNameError, passes after);full
test_utils.pyget_types selection passes, andruff check/ruff formatare clean.