apps: parse log level with logging.getLevelName()
The logging.getLevelName() function returns the numeric representation of logging level.
The args.LEVEL is "warning" by default. Just to be in line with the Python let's make it in upper case "WARNING". But if previously an incorrect level name used e.g. CRIT then the WARNING fallback value used instead, but now it will fail. This shouldn't be a big problem.
Quality Gate passed
Issues
0 New issues
0 Accepted issues
Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code
Raising ValueError (see below) when an unknown loglevel is provided doesn't look appealing to me. So some sort of check is still needed. I would be fine with something like this but at that point, is it really an improvement, maybe...?
if loglevel in ("WARNING", "DEBUG", "INFO"):
logger.setLevel(logging.getLevelName(loglevel))
else:
logger.setLevel(logging.getLevelName("WARNING"))
ValueError
import logging
logger = logging.getLogger()
loglevel = logging.getLevelName("BADLEVEL")
logger.setLevel(loglevel)
Traceback (most recent call last):
File "<pyshell#4>", line 1, in <module>
logger.setLevel(loglevel)
File "/usr/lib/python3.12/logging/__init__.py", line 1514, in setLevel
self.level = _checkLevel(level)
File "/usr/lib/python3.12/logging/__init__.py", line 213, in _checkLevel
raise ValueError("Unknown level: %r" % level)
ValueError: Unknown level: 'Level BADLEVEL'
print(loglevel)
Level BADLEVEL
If a user set a bad loglevel that it will see the error stacktrace, so it can fix it. But how many users do need to set the loglevel? I myself only tried it when tried to debug why my device refuses to connect. And that wasn't useful at all and I watched the bluetooth daemon logs instead. Maybe only developers are interested in the setting the loglevel debug.
If you think that some fallback is better then as a simpler solution we may check that the loglevel is not started from the Level prefix that is returned on unrecognized level name:
https://github.com/python/cpython/blob/main/Lib/logging/init.py#L154C12-L154C18
Unhandled Exceptions are not an option for me. They tend to get caught by reporting tools like abrt. And it only shows the user the application crashed as they don't read the backtrace.
This would be fine as well and informs the user properly what options they have.
diff --git a/blueman/Functions.py b/blueman/Functions.py
index d8810de09..7100738b9 100644
--- a/blueman/Functions.py
+++ b/blueman/Functions.py
@@ -269,8 +269,9 @@ def create_parser(
if parser is None:
parser = argparse.ArgumentParser()
+ debug_choices = ["warning", "debug", "info", "error", "critical"]
if loglevel:
- parser.add_argument("--loglevel", dest="LEVEL", default="warning")
+ parser.add_argument("--loglevel", dest="LEVEL", default="warning", choices=debug_choices)
if syslog:
parser.add_argument("--syslog", dest="syslog", action="store_true")
$ python apps/blueman-services --loglevel bad
usage: blueman-services [-h] [--loglevel {warning,debug,info,error,critical}] [--syslog]
blueman-services: error: argument --loglevel: invalid choice: 'bad' (choose from 'warning', 'debug', 'info', 'error', 'critical')
looks good :+1:
looks good 👍
Can you include the argparse choices in this PR then I'll approve this.