damccorm commented on code in PR #35387: URL: https://github.com/apache/beam/pull/35387#discussion_r2161683527
########## sdks/python/apache_beam/transforms/util.py: ########## @@ -1445,30 +1445,50 @@ class LogElements(PTransform): level: (optional) The logging level for the output (e.g. `logging.DEBUG`, `logging.INFO`, `logging.WARNING`, `logging.ERROR`). If not specified, the log is printed to stdout. + with_pane_info (bool): (optional) Whether to include element's pane info. + use_rfc3339 (bool): (optional) Whether to display timestamps in rfc3339 + format. """ class _LoggingFn(DoFn): def __init__( - self, prefix='', with_timestamp=False, with_window=False, level=None): + self, + prefix='', + with_timestamp=False, + with_window=False, + with_pane_info=False, + use_rfc3339=True, + level=None): Review Comment: Nit: Param order doesn't match order in pydoc ########## sdks/python/apache_beam/transforms/util.py: ########## @@ -1445,30 +1445,50 @@ class LogElements(PTransform): level: (optional) The logging level for the output (e.g. `logging.DEBUG`, `logging.INFO`, `logging.WARNING`, `logging.ERROR`). If not specified, the log is printed to stdout. + with_pane_info (bool): (optional) Whether to include element's pane info. + use_rfc3339 (bool): (optional) Whether to display timestamps in rfc3339 Review Comment: This parameter name is hard to understand, and it is surprising to have one param that defaults to True while the rest default to False. What if we named this `use_epoch_time`? I think people have a better shot of knowing what this means and it lets us keep the defaults to False. -- 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: github-unsubscr...@beam.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org