Skip to content

apps: parse log level with logging.getLevelName()#2555

Open
stokito wants to merge 1 commit intoblueman-project:mainfrom
stokito:parse_loglevel
Open

apps: parse log level with logging.getLevelName()#2555
stokito wants to merge 1 commit intoblueman-project:mainfrom
stokito:parse_loglevel

Conversation

@stokito
Copy link
Copy Markdown

@stokito stokito commented Dec 14, 2024

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.

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.
@sonarqubecloud
Copy link
Copy Markdown

@infirit
Copy link
Copy Markdown
Contributor

infirit commented Dec 16, 2024

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

@stokito
Copy link
Copy Markdown
Author

stokito commented Dec 16, 2024

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

@infirit
Copy link
Copy Markdown
Contributor

infirit commented Dec 16, 2024

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')

@stokito
Copy link
Copy Markdown
Author

stokito commented Dec 16, 2024

looks good 👍

@infirit
Copy link
Copy Markdown
Contributor

infirit commented Dec 18, 2024

looks good 👍

Can you include the argparse choices in this PR then I'll approve this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants