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

refactor: replace the file storage logger with the default logger #311

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

Conversation

jinrenjie
Copy link

I use certmagic in my project, it's great, but I had trouble collecting logs, so I want to replace log with Default.Logger to make the log style uniform!

@mholt
Copy link
Member

mholt commented Sep 16, 2024

Thanks; I suppose this is better than using the std lib logger, as it's the "wrong" logger either way (these places don't have a config available to them for us to get the "correct" logger).

Although, Default is intended as a template and shouldn't be used directly. Plus, it could be modified to set Default.Logger to nil. ( 💥 ) We should probably refer to defaultLogger directly if possible. Would that work?

@jinrenjie
Copy link
Author

@mholt I think Default.Logger is more suitable than defaultLogger, because users can set custom Logger through Default.Logger. In addition, I saw in the code that if Default.Logger is nil, defaultLogger can be assigned to Default.Logger.

@mholt
Copy link
Member

mholt commented Sep 16, 2024

In addition, I saw in the code that if Default.Logger is nil, defaultLogger can be assigned to Default.Logger

That's only the case if a new instance of a Config is made by using Default as an input (like a template).

defaultLogger is the only zap logger guaranteed to be properly set (non-nil).

filestorage.go Outdated Show resolved Hide resolved
Replace Default.Logger with defaultLogger
@jinrenjie
Copy link
Author

@mholt I have replaced Default.Logger with defaultLogger.

@mholt
Copy link
Member

mholt commented Dec 12, 2024

Thanks -- sorry I've been backlogged. This is still on my list just FYI :)

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.

3 participants