-
Notifications
You must be signed in to change notification settings - Fork 36
Set log level from config or cli args #528
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
Conversation
multiversx_sdk_cli/cli.py
Outdated
| args.insert(0, verbose_arg) | ||
|
|
||
|
|
||
| def _handle_log_level_argument(args: list[str]): |
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.
Could have handled both --verbose and --log-level in a single function of the kind e.g. _handle_global_arguments or _handle_universal_arguments. Just an opinion.
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.
done, moved the logic in _handle_global_arguments().
| parser.add_argument( | ||
| "--log-level", | ||
| type=str, | ||
| default=config.get_log_level_from_config(), |
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.
👍
multiversx_sdk_cli/config.py
Outdated
|
|
||
| def get_log_level_from_config(): | ||
| log_level = get_value("log_level") | ||
| if log_level not in ["debug", "info", "warning", "error"]: |
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.
List could have been in constants.
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.
done
multiversx_sdk_cli/errors.py
Outdated
|
|
||
| class LogLevelError(KnownError): | ||
| def __init__(self, log_level: str): | ||
| super().__init__(f"Log level not accepted: {log_level}. Choose between ['debug', 'info', 'warning', 'error'].") |
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.
List could have been in constants.
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.
done
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.
Pull Request Overview
This PR adds functionality to configure the logging level via the configuration file or through the --log-level CLI argument. Key changes include:
- Introducing a new LogLevelError in errors.py.
- Adding a get_log_level_from_config function to ensure the log level is valid in config.py.
- Updating cli.py to parse and reorder the --log-level argument, and applying the log level during logging initialization.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| multiversx_sdk_cli/errors.py | Adds LogLevelError to error handling with a clear message for invalid log levels. |
| multiversx_sdk_cli/config.py | Sets a default log_level and adds a helper to fetch and validate its value. |
| multiversx_sdk_cli/cli.py | Adds support for the --log-level CLI argument and reorders arguments accordingly. |
| log_level_arg = "--log-level" | ||
|
|
||
| if log_level_arg in args: | ||
| index = args.index(log_level_arg) |
Copilot
AI
Jun 18, 2025
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.
Consider adding a safety check to ensure that a value follows '--log-level' in the args list to prevent potential index errors if the argument is provided without an accompanying value.
| index = args.index(log_level_arg) | |
| index = args.index(log_level_arg) | |
| if index + 1 >= len(args) or args[index + 1].startswith("--"): | |
| raise ValueError(f"Argument '{log_level_arg}' must be followed by a log level value.") |
| if log_level not in LOG_LEVELS: | ||
| default_log_level = get_defaults()["log_level"] | ||
| show_warning(f"Invalid log level set in config: [{log_level}]. Defaulting to [{default_log_level}].") | ||
| return default_log_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.
Mypy warning.
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.
fixed
An user can set the
log_levelconfig entry usingmxpy config set log_level error. The allowed values are [debug, info, warning, error]. The default value isinfo.Additionally, the
--log-levelargument can be provided with anymxpycommand. Explicitly specifying this argument will override the value set in the config.