tvalentyn commented on a change in pull request #14815:
URL: https://github.com/apache/beam/pull/14815#discussion_r632693594
##########
File path: sdks/python/apache_beam/transforms/display.py
##########
@@ -137,9 +137,12 @@ def create_payload(dd):
except ValueError:
# Skip if the display data is invalid.
return None
- if 'value' not in display_data_dict or 'label' not in display_data_dict:
+ if 'value' not in display_data_dict or (
Review comment:
1. should this be `if 'value' not in display_data_dict` ***and*** ... ?
I think you could rewrite it as:
```
try:
label = (
display_data_dict['label']
if 'label' in display_data_dict else display_data_dict['key'])
value = display_data_dict['value']
except KeyError:
return None
```
2. Where is the expected structure of display_data_dict documented? Why
these particular keys in DD dictionary have specific treatment? Should we
assert that `label` and `key` don't appear at the same time?
3. should we update display_test as well?
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
