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

[RFC] Allow to configure exception formatter #86

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented Apr 9, 2019

TODO:

Came across this when looking at #59, i.e. when you want to be compatible to logging, no exceptions should be added also.

@Delgan
Copy link
Owner

Delgan commented Apr 9, 2019

I'm not sure to follow what is wrong with #59, is there a problem with the Pytest workaround provided? Some exceptions that are displayed while they should not?

Thanks for the suggestion but I don't want exception_formatter formatter to be part of the API. I don't expect custom exception formatting to be a very common use case, and it's currently already possible with just a few lines of code:

def formatter(record):
    fmt = "{time} {name} - {message}"
    if record["exception"] is not None:
        record["extra"]["formatted_exception"] = my_exception_formatter(record["exception"])
        fmt += " {extra[formatted_exception]}\n"
    return fmt

logger.add(sys.stderr, format=formatter)

@blueyed
Copy link
Contributor Author

blueyed commented Apr 9, 2019

I'm not sure to follow what is wrong with #59, is there a problem with the Pytest workaround provided? Some exceptions that are displayed while they should not?

It is not really compatible from what I can tell, since exceptions are added to the message - this at least does not really work when migrating from logging to loguru. (However I might have understood the intention there)

As for this PR I've just thought that processing could be skipped in general if not required for example.

I don't want exception_formatter formatter to be part of the API

I see.. then I will look into removing this from here, IIRC there are other useful bits in it.
My main motivation was to not have a custom one, but allow to disable it.

Your example code looks like it would do the job, but is a bit more involved (but fine).
It would still instantiate the internal formatter though, wouldn't it?

@Delgan
Copy link
Owner

Delgan commented Apr 12, 2019

It is not really compatible from what I can tell, since exceptions are added to the message - this at least does not really work when migrating from logging to loguru. (However I might have understood the intention there)

Ok, yes, I see, sorry. This is because the formatter in logger.add(sink, format="{message}") is not simply "{message}" but actually "{message}\n{exception}", is that what you mean?

So, when logging.makeRecord() is called to create a record compatible with standard logging, the .msg attribute of the resulting LogRecord object will potentially contain the formatted exception, while it shouldn't. Hence the exc_text = "\n" hack that I added to prevent the exception to be displayed twice.
I'm starting remembering all these nasty things I have done... :)

My main motivation was to not have a custom one, but allow to disable it.

Your example code looks like it would do the job, but is a bit more involved (but fine).

The "\n{exception}" is automatically added for convenience, but there is actually a way to disable it by using a formatter function: logger.add(sink, format=lambda _: "{message}\n").

I think this would produce the same result that your proposition.

However, this implies that logger.exception("Error") would not propagate the traceback. Actually, the LogRecord.msg contains the log message formatted by Loguru, whether it is "{message}", "{message}\n{exception}", "{time} {function} {message}", etc. That's way, the user can choose the way it prefer to format the propagated message (through standard setFormatter() or Loguru format).

It would still instantiate the internal formatter though, wouldn't it?

Yes, but I'm not worried about that. The exception formatter is instantiated only once, while adding the handler, it's not a problem. When calling logger.exception(), the exception will be formatted but not added to the message, though, which I agree is a waste of resources (but can't be easily fixed without extending the Logger api).

@blueyed
Copy link
Contributor Author

blueyed commented Apr 12, 2019

but can't be easily fixed without extending the Logger api

You could set it via private methods directly.

Otherwise your information is what I've seen also.

My plan for an improved caplog fixture would be to e.g. use a raw handler and make that available also. Haven't looked into it more yet though.

@Delgan
Copy link
Owner

Delgan commented Apr 13, 2019

You could set it via private methods directly.

How could I? It would require to inspect the format in order to detect if it contains "{exception}", but I prefer to avoid that kind of parsing.

My plan for an improved caplog fixture would be to e.g. use a raw handler and make that available also. Haven't looked into it more yet though.

I don't know exactly what you have in mind, improvements are much welcome, as long as they don't extend the api just for uncommon use cases. 😉

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