gemini-code-assist[bot] commented on code in PR #35778:
URL: https://github.com/apache/beam/pull/35778#discussion_r2251344904
##########
sdks/python/apache_beam/options/pipeline_options.py:
##########
@@ -1942,6 +1942,12 @@ def _add_argparse_args(cls, parser):
'downloading a zipped prism binary, for the current platform. If '
'prism_location is set to a Github Release page URL, them it will use '
'that release page as a base when constructing the download URL.')
+ parser.add_argument(
+ '--prism_log_level',
+ default="info",
+ help=(
+ 'Controls the log level in Prism. Values can be "debug", "info", '
+ '"warn", and "error". Default log level is "info".'))
Review Comment:
Consider using the `choices` parameter in `argparse.add_argument` for better
validation and maintainability. This provides immediate feedback to the user
for invalid values and is consistent with other options in this file.
Using `choices` will make the option case-sensitive. The help string already
lists the options in lowercase, so this seems acceptable.
This change would also make the validation in `_get_prism_log_level()` in
`prism_runner.py` redundant, allowing for its simplification or removal.
```suggestion
parser.add_argument(
'--prism_log_level',
default="info",
choices=["debug", "info", "warn", "error"],
help=(
'Controls the log level in Prism. Values can be "debug", "info",
'
'"warn", and "error". Default log level is "info".'))
```
##########
sdks/python/apache_beam/runners/portability/prism_runner.py:
##########
@@ -111,6 +111,19 @@ def _rename_if_different(src, dst):
os.rename(src, dst)
+def _get_prism_log_level(log_level: str) -> str:
+ """Returns a validated log level for Prism.
+
+ The accepted values are "debug", "info", "warn", and "error".
+ """
+ if log_level.lower() in ["debug", "info", "warn", "error"]:
+ return log_level.lower()
+
+ raise ValueError(
+ f'Unknown prism log level: "{log_level}". '
+ 'Available values are "debug", "info", "warn", and "error".')
Review Comment:
This validation logic can be improved for maintainability by defining the
valid log levels as a constant set. This avoids repeating the list of strings
in the function and in the error message, making it easier to update in the
future.
As noted in another comment, if `choices` is added to the `argparse`
definition for this option, this validation becomes redundant. If that change
is made, this function can be removed entirely and
`prism_options.prism_log_level` can be used directly in
`PrismJobServer.__init__`.
If you prefer to keep the case-insensitive validation here, here is a
suggested improvement:
```suggestion
def _get_prism_log_level(log_level: str) -> str:
"""Returns a validated log level for Prism.
The accepted values are "debug", "info", "warn", and "error".
"""
valid_levels = {"debug", "info", "warn", "error"}
lower_log_level = log_level.lower()
if lower_log_level in valid_levels:
return lower_log_level
raise ValueError(
f'Unknown prism log level: "{log_level}". '
f'Available values are: {", ".join(sorted(valid_levels))}.')
```
--
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
