Modify Config during Error Checking #1341
Conversation
pgierz
requested review from
Copilot and
seb-wahl
and removed request for
Copilot
| frequency = int(frequency) | ||
| except: | ||
| frequency = 60 | ||
| if ( |
There was a problem hiding this comment.
If only one of 'set_config_key' or 'set_config_value' is provided, the config update is silently skipped. Consider adding logging or a warning to help debug misconfigurations.
There was a problem hiding this comment.
Logging would be helpful, that is a good suggestion...
| "PyGithub==1.55", | ||
| "colorama==0.4.5", | ||
| "coloredlogs==15.0.1", # NOTE(PG): Should be removed during cleanup for loguru instead | ||
| "dpath>=2.0.7", # Maximum version for Python 3.6 support |
There was a problem hiding this comment.
The version specifier '>=2.0.7' contradicts the comment stating a maximum version is required for Python 3.6 support. Consider revising the version constraint (e.g., using '<=' if a maximum version is intended).
| "dpath>=2.0.7", # Maximum version for Python 3.6 support | |
| "dpath>=2.0.7, <2.1.0", # Maximum version for Python 3.6 support |
There was a problem hiding this comment.
This is a bad suggestion, or maybe an indication of bad working in my comment: we need 2.0.7 or newer (2.0.7 is the highest version still supports Python 3.6)
There was a problem hiding this comment.
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/esm_runscripts/observe.py:144
- Using a bare except can mask unexpected errors. Consider catching a specific exception to improve error handling clarity.
except:
| frequency = int(frequency) | ||
| except: | ||
| frequency = 60 | ||
| if ( |
There was a problem hiding this comment.
The variables 'set_config_key' and 'set_config_value' are defined conditionally. To ensure they are always available later in the function, consider initializing them to None before the conditional block.
There was a problem hiding this comment.
This is done below, so the comment is not needed.
This allows the error checker to modify config values. It adds a new dependency
dpathto the project.Untested, but should work...