Skip to content
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

Feature request: Add except exceptions list to catch decorator. #248

Closed
heckad opened this issue Apr 16, 2020 · 10 comments
Closed

Feature request: Add except exceptions list to catch decorator. #248

heckad opened this issue Apr 16, 2020 · 10 comments
Labels
feature Request for adding a new feature

Comments

@heckad
Copy link

heckad commented Apr 16, 2020

For some reason, my function has special exception signals, and I don’t want to catch them. Have a filter will be helpful.

@Delgan
Copy link
Owner

Delgan commented Apr 17, 2020

Hey @heckad.

The next Loguru release will feature a new onerror parameter for the logger.catch() decorator, I think you will be able to use it to filter-out some exceptions.

However, I'm not sure I want to add too much functionality to this method. It should remain a very basic decorator whose main purpose is to catch unexpected exceptions. In particular, it should ideally not alter the control flow of a program.

Good old try / except is maybe more appropriate in your case. Do you have any example of what you would like to do with the exception not being caught?

@heckad
Copy link
Author

heckad commented Apr 17, 2020

from loguru import logger


class StopConsumeNewValue(Exception):
    pass


def many_functions_call_later(value):
    if value == 5:
        raise StopConsumeNewValue


@logger.catch(exclude_exceptions=StopConsumeNewValue)
def handle_new_value(value):
    many_functions_call_later(value)


def consume_event():
    try:
        while True:
            new_event = get_new_event()

            handle_new_value(new_event.value)
    except StopConsumeNewValue:
        pass

In function many_functions_call_later I mean that this function call many other functions in deep and stack may be very big. For this reason, an exception is a good way. Your solution is also good. You can add ignore function which means that ignore some exceptions. Example of use

@logger.catch(onerror=ignre(StopConsumeNewValue))
def handle_new_value(value):
    many_functions_call_later(value)

@Delgan
Copy link
Owner

Delgan commented Apr 18, 2020

Thanks for the code sample. 👍

I don't know. On the one hand, I like your suggestion to add an exclude=StopConsumeNewValue argument: it's elegant, simple and it would solve your problem perfectly.

On the other hand, I'm not sure it's "good practice". As I said, I think it's important that logging remains separate from the control flow of the program. By adding an exclude argument, we are basically creating syntactic sugar to replace:

try:
    handle_new_value(new_event.value)
except StopConsumeNewValue:
    pass
except Exception:
    logger.exception("Unexpected error")

I'm mostly worried about the case where the function returns a value. If one day you decide to return something from handle_new_value(), you can extend the above code painlessly.

try:
    result = handle_new_value(new_event.value)
except StopConsumeNewValue:
    pass
except Exception:
    logger.exception("Unexpected error")
else:
    logger.info("Result: {}", result)

Using @logger.catch(), you would have to check whether or not the returned value is None which doesn't seem right.

try:
    result = handle_new_value(new_event.value)
except StopConsumeNewValue:
    pass
else:
    if result is not None:
        logger.info("Result: {}", result)

Of course, someone could argue that it's already the case, even without adding the new exclude argument. That's why I think if the user knows that a function can possibly raise an exception or return a value, he should handle this manually. I'm not sure @logger.catch() should be used for anything besides wrapping main() or Thread.run(). 😕

@Delgan Delgan added the feature Request for adding a new feature label Apr 18, 2020
@heckad
Copy link
Author

heckad commented Apr 18, 2020

Please add ability to control raise an exception or not to onerror callback. For example use

@logger.catch(onerror=lambda e: isinstance(e, StopConsumeNewValue))
def f():
    ...

Implementation example
Instead of

if onerror is not None:
    onerror(value)

return not reraise

do something like

if onerror is not None:
    reraise_from_callback = onerror(value)
    if reraise_from_callback is not None:
        if reraise:
             return not reraise_from_callback or
        else;
             return True

return not reraise

onerror should return Optional[bool].
Idea description: If reraise is False then ignore return from onerror callback else checks callback result. If True then ignore else raise an error.

@Delgan
Copy link
Owner

Delgan commented Apr 18, 2020

It's something I've been thinking about, but it's not a good idea in my opinion. It's too implicit and not what one expects with a function named like this. The purpose of onerror is not to decide whether or not the exception should be raised, that's the purpose of reraise.

Another solution is to "silently" catch exception if message=None:

@logger.catch
@logger.catch(StopConsumeNewValue, message=None)
def f():
    ...

However, this means non-trivial exception handling need to be shifted to onerror which is not ideal.

@heckad
Copy link
Author

heckad commented Apr 18, 2020

Maybe add to reraise ability to use callable. Example of implementation

if callable(reraise):
    return reraise(value)
else:
    return value

It's simple and doesn’t look bad.

@Delgan
Copy link
Owner

Delgan commented Apr 18, 2020

If I were to implement it, I would choose the exclude or ignore argument since it's the most straightforward. It's just I don't know yet if it's worth it and it's not the wrong way to solve your problem.

@heckad
Copy link
Author

heckad commented Apr 19, 2020

Also, it's needed for a flask app. Flask recommends use abort function which raise HTTPException wich not needed to catch.

@Delgan
Copy link
Owner

Delgan commented Apr 20, 2020

All right, you won. I added the new exclude argument. 😄

It was very simple to implement and it doesn't complicate the API. Since it can be useful according to the use cases you reported, let's just add it.

Thanks for the feature suggestion. 👍

@Delgan Delgan closed this as completed Apr 20, 2020
@heckad
Copy link
Author

heckad commented Apr 20, 2020

Thanks for add new feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Request for adding a new feature
Projects
None yet
Development

No branches or pull requests

2 participants