diff --git a/fire/helptext.py b/fire/helptext.py index 347278da..f20a3d65 100644 --- a/fire/helptext.py +++ b/fire/helptext.py @@ -491,7 +491,8 @@ def _CreateFlagItem(flag, docstring_info, spec, required=False, # We need to handle the case where there is a default of None, but otherwise # the argument has another type. - if arg_default == 'None': + # Don't wrap in Optional if the type already indicates it's optional. + if arg_default == 'None' and arg_type and 'None' not in arg_type: arg_type = f'Optional[{arg_type}]' arg_type = f'Type: {arg_type}' if arg_type else '' @@ -524,6 +525,11 @@ def _GetArgType(arg, spec): """ if arg in spec.annotations: arg_type = spec.annotations[arg] + # For generic types (e.g., Optional[str], List[int], Union[int, str]), + # __qualname__ only returns the base name without type arguments. + # Use repr() instead to get the full type string. + if hasattr(arg_type, '__args__'): + return repr(arg_type) try: return arg_type.__qualname__ except AttributeError: diff --git a/fire/helptext_test.py b/fire/helptext_test.py index c7098fc4..9fb2dd39 100644 --- a/fire/helptext_test.py +++ b/fire/helptext_test.py @@ -152,6 +152,37 @@ def testHelpTextFunctionWithTypesAndDefaultNone(self): help_screen) self.assertNotIn('NOTES', help_screen) + def testHelpTextOptionalTypeWithDefault(self): + # Regression test for https://github.com/google/python-fire/issues/508 + # Optional[str] with a non-None default should show the full type + # including the inner type, not just "Optional" or "Union". + component = tc.py3.WithDefaultsAndTypes().get_optional_str + help_screen = helptext.HelpText( + component=component, + trace=trace.FireTrace(component, name='get_optional_str')) + self.assertIn('Type:', help_screen) + # The type annotation must include 'str' to indicate the actual type. + # Without the fix, __qualname__ returns just 'Optional' or 'Union' + # which loses the inner type information. + self.assertRegex(help_screen, r'Type:.*str') + # Must not show bare 'Optional' or 'Union' without type arguments + self.assertNotRegex(help_screen, r'Type: Optional\n') + self.assertNotRegex(help_screen, r'Type: Union\n') + + def testHelpTextOptionalTypeWithNoneDefault(self): + # Regression test for https://github.com/google/python-fire/issues/508 + # Optional[str] with default=None should NOT show "Optional[Optional]" + # or "Optional[Union]". + component = tc.py3.WithDefaultsAndTypes().get_optional_str_none + help_screen = helptext.HelpText( + component=component, + trace=trace.FireTrace(component, name='get_optional_str_none')) + self.assertIn('Type:', help_screen) + self.assertNotIn('Optional[Optional]', help_screen) + self.assertNotIn('Optional[Union]', help_screen) + # Should show the inner type (str) in the type annotation + self.assertRegex(help_screen, r'Type:.*str') + def testHelpTextFunctionWithTypes(self): component = tc.py3.WithTypes().double help_screen = helptext.HelpText( diff --git a/fire/test_components_py3.py b/fire/test_components_py3.py index 192302d3..8d669172 100644 --- a/fire/test_components_py3.py +++ b/fire/test_components_py3.py @@ -16,6 +16,7 @@ import asyncio import functools +import typing from typing import Tuple @@ -99,3 +100,9 @@ def double(self, count: float = 0) -> float: def get_int(self, value: int = None): return 0 if value is None else value + + def get_optional_str(self, value: typing.Optional[str] = 'something'): + return value + + def get_optional_str_none(self, value: typing.Optional[str] = None): + return value