jrmccluskey commented on code in PR #28680:
URL: https://github.com/apache/beam/pull/28680#discussion_r1339140947
##########
sdks/python/apache_beam/transforms/core.py:
##########
@@ -1484,8 +1490,13 @@ def __init__(self, fn, *args, **kwargs):
'Using yield and return in the process method '
'of %s can lead to unexpected behavior, see:'
'https://github.com/apache/beam/issues/22969.',
- self.fn.__class__)
+ self.fn.__class__)
+ elif _check_fn_use_yield_and_return(self.fn.process) is None:
+ _LOGGER.warning(
+ 'no iterator is returned by the process method in %s',
+ self.fn.__class__)
+
Review Comment:
Mechanically you probably don't want to re-run
`_check_fn_use_yield_and_return()` here. Consider capturing the output value
the first time and using that here instead
##########
sdks/python/apache_beam/transforms/core.py:
##########
@@ -1423,15 +1423,21 @@ def _check_fn_use_yield_and_return(fn):
source_code = _get_function_body_without_inners(fn)
has_yield = False
has_return = False
+ if len(source_code)==0:
+ return None
Review Comment:
What is the purpose of this check?
##########
sdks/python/apache_beam/transforms/core.py:
##########
@@ -1423,15 +1423,21 @@ def _check_fn_use_yield_and_return(fn):
source_code = _get_function_body_without_inners(fn)
has_yield = False
has_return = False
+ if len(source_code)==0:
+ return None
for line in source_code.split("\n"):
if line.lstrip().startswith("yield ") or line.lstrip().startswith(
"yield("):
has_yield = True
if line.lstrip().startswith("return ") or line.lstrip().startswith(
"return("):
has_return = True
+ if line.lstrip().startswith("return None"):
Review Comment:
The warn could also be placed here instead, eliminating the need for the
`elif` block.
--
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]
