-
Notifications
You must be signed in to change notification settings - Fork 12
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
Fix for non-working log_on_error due to bad scope of exceptions. Remove unused value from log_exception #20
base: master
Are you sure you want to change the base?
Conversation
@sighalt cannot add You as a reviewer |
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.
Thank you very much for contributing. Aside from the actual code changes, could you also provide a test for the problem?
@@ -170,5 +170,5 @@ def __init__(self, message: str, *, logger: Optional[Logger] = None, | |||
exception_format_variable=exception_format_variable) | |||
|
|||
@staticmethod | |||
def log(logger: Logger, log_level: Union[str, int], msg: str) -> None: | |||
def log(logger: Logger, msg: str) -> None: |
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.
This change breaks the signature of the parent's log
method. I rather have the unused argument.
@@ -145,7 +145,7 @@ def _do_logging(self, fn: FunctionType, exception: Exception, *args: Any, **kwar | |||
def execute(self, fn: FunctionType, *args: Any, **kwargs: Any) -> Any: | |||
try: | |||
return super().execute(fn, *args, **kwargs) | |||
except Exception as e: | |||
except BaseException as e: |
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.
Nice catch! Could you also change the type annotations of on_error
and _do_logging
, so they require BaseException
instead of Exception
?
You can check if everything is alright with mypy -p logdecorator
.
Sure. I'll try to provide some unit tests according to your current approach |
Hi. I just recognized that @log_on_error throws an exception and doesn't give any log. So I found what's the problem - Exception, so I changed it to the BaseException. After that it catches the exception from function and outputs the log msg :)