jrmccluskey commented on code in PR #37645:
URL: https://github.com/apache/beam/pull/37645#discussion_r2828419251
##########
sdks/python/apache_beam/typehints/typehints_test.py:
##########
@@ -1596,6 +1596,25 @@ def test_hint_helper(self):
self.assertFalse(is_consistent_with(Union[str, int], str))
self.assertFalse(is_consistent_with(str, NonBuiltInGeneric[str]))
+ def test_hint_helper_pipe_union(self):
+ # GH issue: https://github.com/apache/beam/issues/36592
+ # Python 3.10+ pipe operator union types (types.UnionType) should work
+ # in is_consistent_with just like typing.Union.
Review Comment:
You don't need to reference a github issue that you're resolving in the code
##########
sdks/python/apache_beam/typehints/typehints.py:
##########
@@ -1462,6 +1462,12 @@ def normalize(x, none_as_type=False):
# Convert bare builtin types to correct type hints directly
elif x in _KNOWN_PRIMITIVE_TYPES:
return _KNOWN_PRIMITIVE_TYPES[x]
+ elif isinstance(x, types.UnionType):
+ beam_type = native_type_compatibility.convert_to_beam_type(x)
+ if beam_type != x:
+ return beam_type
+ else:
+ return Any
Review Comment:
Adding the extra conditional here and repeating the same code as the other
categories that get converted is clunky. Now that the list of checks is
reasonably long, it may be worth splitting them out into a helper function for
readability; otherwise, `isinstance(x, types.UnionType)` should be worked into
the existing elif statement.
##########
sdks/python/apache_beam/typehints/typehints_test.py:
##########
@@ -1934,6 +1953,16 @@ def test_pipe_operator_as_union(self):
native_type_compatibility.convert_to_beam_type(type_a),
native_type_compatibility.convert_to_beam_type(type_b))
+ def test_normalize_pipe_union(self):
+ # GH issue: https://github.com/apache/beam/issues/36592
+ # normalize() should convert types.UnionType to Beam's UnionConstraint.
Review Comment:
Same thing here, don't need to link the issue
##########
sdks/python/apache_beam/typehints/typehints_test.py:
##########
@@ -1596,6 +1596,25 @@ def test_hint_helper(self):
self.assertFalse(is_consistent_with(Union[str, int], str))
self.assertFalse(is_consistent_with(str, NonBuiltInGeneric[str]))
+ def test_hint_helper_pipe_union(self):
Review Comment:
Having the extra test coverage is nice but it's worth noting that this suite
passes even without the changes to typehints.py
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
