Skip to content

Commit 9a07c80

Browse files
mstoclaude
andcommitted
fix: recognize PEP 604 union syntax in _attribute_is_optional
Closes #293. `_attribute_is_optional` only checked `typing.get_origin(attribute.type) is Union`, so Metric and attr_from classes defined with PEP 604 annotations (`T | None`) treated optional fields as required. Mirror the dual-origin check already used by `_is_optional` in fgpyo/util/types.py. Also folds in the deferred #286 migrations in test_metric.py (7 sites) and test_inspect.py (3 sites), which exercise this code path. A new LegacyOptionalName attr class and two tests keep coverage of the legacy `Union` / `Optional` syntax branch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent bf9981e commit 9a07c80

3 files changed

Lines changed: 40 additions & 11 deletions

File tree

fgpyo/util/inspect.py

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -491,7 +491,8 @@ def attr_from(
491491

492492
def _attribute_is_optional(attribute: FieldType) -> bool:
493493
"""Returns True if the attribute is optional, False otherwise."""
494-
return typing.get_origin(attribute.type) is Union and isinstance(
494+
origin = typing.get_origin(attribute.type)
495+
return (origin is Union or origin is python_types.UnionType) and isinstance(
495496
None, typing.get_args(attribute.type)
496497
)
497498

tests/fgpyo/util/test_inspect.py

Lines changed: 31 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
from typing import Optional
55
from typing import Set
66
from typing import Tuple
7+
from typing import Union
78

89
import attr
910
import pytest
@@ -28,9 +29,23 @@ class Name:
2829
required: str
2930
custom_parser: str
3031
converted: int = attr.field(converter=int)
32+
optional_no_default: str | None
33+
optional_with_default_none: str | None = None
34+
optional_with_default_some: str | None = "foo"
35+
36+
37+
@attr.s(auto_attribs=True, frozen=True)
38+
class LegacyOptionalName:
39+
"""
40+
Test attr class using legacy `Optional[T]` / `Union[T, None]` syntax.
41+
42+
Kept to ensure `_attribute_is_optional` continues to recognize the
43+
`typing.Union` origin alongside `types.UnionType` (PEP 604).
44+
"""
45+
46+
required: str
3147
optional_no_default: Optional[str]
32-
optional_with_default_none: Optional[str] = None
33-
optional_with_default_some: Optional[str] = "foo"
48+
union_with_none: Union[str, None] = None
3449

3550

3651
def test_attr_from() -> None:
@@ -57,6 +72,13 @@ def test_attribute_is_optional() -> None:
5772
assert _attribute_is_optional(fields_dict["optional_with_default_some"])
5873

5974

75+
def test_attribute_is_optional_legacy_syntax() -> None:
76+
fields_dict = attr.fields_dict(LegacyOptionalName)
77+
assert not _attribute_is_optional(fields_dict["required"])
78+
assert _attribute_is_optional(fields_dict["optional_no_default"])
79+
assert _attribute_is_optional(fields_dict["union_with_none"])
80+
81+
6082
def test_attribute_has_default() -> None:
6183
fields_dict = attr.fields_dict(Name)
6284
assert not _attribute_has_default(fields_dict["required"])
@@ -67,6 +89,13 @@ def test_attribute_has_default() -> None:
6789
assert _attribute_has_default(fields_dict["optional_with_default_some"])
6890

6991

92+
def test_attribute_has_default_legacy_syntax() -> None:
93+
fields_dict = attr.fields_dict(LegacyOptionalName)
94+
assert not _attribute_has_default(fields_dict["required"])
95+
assert _attribute_has_default(fields_dict["optional_no_default"])
96+
assert _attribute_has_default(fields_dict["union_with_none"])
97+
98+
7099
class Foo:
71100
"""Test class used as a field type."""
72101

tests/fgpyo/util/test_metric.py

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@
1212
from typing import Callable
1313
from typing import Dict
1414
from typing import List
15-
from typing import Optional
1615
from typing import Set
1716
from typing import Tuple
1817
from typing import Type
@@ -108,8 +107,8 @@ class DummyMetric(Metric["DummyMetric"]):
108107

109108
@make_dataclass(use_attr=use_attr)
110109
class Person(Metric["Person"]):
111-
name: Optional[str]
112-
age: Optional[int]
110+
name: str | None
111+
age: int | None
113112

114113
@make_dataclass(use_attr=use_attr)
115114
class Name:
@@ -145,7 +144,7 @@ def format_value(cls, value: Any) -> str:
145144
@make_dataclass(use_attr=use_attr)
146145
class PersonMaybeAge(Metric["PersonMaybeAge"]):
147146
name: str
148-
age: Optional[int]
147+
age: int | None
149148

150149
@make_dataclass(use_attr=use_attr)
151150
class PersonDefault(Metric["PersonDefault"]):
@@ -154,13 +153,13 @@ class PersonDefault(Metric["PersonDefault"]):
154153

155154
@make_dataclass(use_attr=use_attr)
156155
class PersonAgeFloat(Metric["PersonAgeFloat"]):
157-
name: Optional[str]
158-
age: Optional[float]
156+
name: str | None
157+
age: float | None
159158

160159
@make_dataclass(use_attr=use_attr)
161160
class ListPerson(Metric["ListPerson"]):
162-
name: List[Optional[str]]
163-
age: List[Optional[int]]
161+
name: List[str | None]
162+
age: List[int | None]
164163

165164
self.DummyMetric = DummyMetric
166165
self.Person = Person

0 commit comments

Comments
 (0)