-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update supported Python versions and update CI/CD #35
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot Diego! LGTM, I left only a couple of minor comments
src/rod/__init__.py
Outdated
def get_default_logging_level(env_var: str) -> logging.LoggingLevel: | ||
""" | ||
Get the default logging level. | ||
|
||
Args: | ||
env_var: The environment variable to check. | ||
|
||
Returns: | ||
The logging level to set. | ||
""" | ||
|
||
import os | ||
|
||
try: | ||
return logging.LoggingLevel[os.environ[env_var].upper()] | ||
|
||
# Raise if the environment variable is set but the logging level is invalid. | ||
except AttributeError: | ||
msg = f"Invalid logging level defined in {env_var}: '{os.environ[env_var]}'" | ||
raise ValueError(msg) | ||
|
||
# If the environment variable is not set, return the logging level depending | ||
# on the installation mode. | ||
except KeyError: | ||
return ( | ||
logging.LoggingLevel.DEBUG | ||
if installation_is_editable() | ||
else logging.LoggingLevel.WARNING | ||
) | ||
|
||
|
||
# Configure the logger with the default logging level. | ||
logging.configure(level=get_default_logging_level(env_var="ROD_LOGGING_LEVEL")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the environment variable can only be ROD_LOGGING_LEVEL
, is there a reason why you're passing it as the argument of this function? Can it just be hardcoded?
def get_default_logging_level(env_var: str) -> logging.LoggingLevel: | |
""" | |
Get the default logging level. | |
Args: | |
env_var: The environment variable to check. | |
Returns: | |
The logging level to set. | |
""" | |
import os | |
try: | |
return logging.LoggingLevel[os.environ[env_var].upper()] | |
# Raise if the environment variable is set but the logging level is invalid. | |
except AttributeError: | |
msg = f"Invalid logging level defined in {env_var}: '{os.environ[env_var]}'" | |
raise ValueError(msg) | |
# If the environment variable is not set, return the logging level depending | |
# on the installation mode. | |
except KeyError: | |
return ( | |
logging.LoggingLevel.DEBUG | |
if installation_is_editable() | |
else logging.LoggingLevel.WARNING | |
) | |
# Configure the logger with the default logging level. | |
logging.configure(level=get_default_logging_level(env_var="ROD_LOGGING_LEVEL")) | |
def get_default_logging_level() -> logging.LoggingLevel: | |
""" | |
Get the default logging level. | |
Returns: | |
The logging level to set. | |
""" | |
import os | |
try: | |
return logging.LoggingLevel[os.environ["ROD_LOGGING_LEVEL"].upper()] | |
# Raise if the environment variable is set but the logging level is invalid. | |
except AttributeError: | |
msg = f"Invalid logging level defined in 'ROD_LOGGING_LEVEL'" | |
raise ValueError(msg) | |
# If the environment variable is not set, return the logging level depending | |
# on the installation mode. | |
except KeyError: | |
return ( | |
logging.LoggingLevel.DEBUG | |
if installation_is_editable() | |
else logging.LoggingLevel.WARNING | |
) | |
# Configure the logger with the default logging level. | |
logging.configure(level=get_default_logging_level() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having the environment variable explicitly defined makes it more visible to users rather than hiding it inside the logic of a function. Since we do not have documentation, I valued readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes sense, thanks for the explanation!
37d1227
to
5e96179
Compare
Co-authored-by: Filippo Luca Ferretti <102977828+flferretti@users.noreply.github.com>
FYI @lorycontixd I had to update your test introduced in #33 to make it work on Windows. |
@diegoferigo Yep, no problem. |
No description provided.