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

Fix: New log formats not being displayed #241

Merged
merged 1 commit into from
Mar 9, 2024

Conversation

shtlrs
Copy link
Contributor

@shtlrs shtlrs commented Jan 19, 2024

As I was testing the image on my local cluster, i noticed that the formatting I've setup in my previous PR hasn't taken effect.

This is because when we ask for a new logger, it doesn't come with any handlers.

The solution to this is to use logging.basicConfig upon package instantiation.

I know that we previously discussed this, but I said it was a problem if we put it in __main__.py as it'll only be available when the server is launched, and not if it's used as a package. And I don't know why I didn't think about putting it in the package's __init__.py .

@supakeen I know i've kep the get_logger method even though it does practically nothing.
I just wanted to reduce the changes and also let room for extra setup if we're ever going to do that.
Also, what do you think about making the log level configurable ?

image

This is done by exposing the setup_logging method that applies the formatting & log level
@supakeen
Copy link
Owner

@shtlrs It should already be configurable, you can call setup_logging from here: https://github.com/supakeen/pinnwand/blob/master/src/pinnwand/command.py#L30

@shtlrs
Copy link
Contributor Author

shtlrs commented Jan 23, 2024

@shtlrs It should already be configurable, you can call setup_logging from here: https://github.com/supakeen/pinnwand/blob/master/src/pinnwand/command.py#L30

@supakeen it's only configurable via the CLI there. Not when used as a package.

What i meant to say is, we can make it configurable via a debug that'll log at debug level when developing and at INFO level (min) in production.
Then if someone wants to suppress that, they could using that option.

@supakeen supakeen merged commit 0965564 into supakeen:master Mar 9, 2024
10 checks passed
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