Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

my.core.logging: compatibility with HPI_LOGS #307

Merged
merged 1 commit into from
Sep 7, 2023

Conversation

purarue
Copy link
Contributor

@purarue purarue commented Sep 6, 2023

I had tried to set the DEFAULT_LEVEL like we discussed in #305 here:

DEFAULT_LEVEL = 'INFO'

But if the module had already set a level like:

from my.core import make_logger

logger = make_logger(__name__, level="warning")

then any attempt to use HPI_LOGS (which would use the DEFAULT_LEVEL) would be ignored, which is not what the user would expect (they'd expect the --debug flag to just override everything)

So, added a check after the LOGGING_LEVEL_ check in the get_env_level function instead

@karlicoss
Copy link
Owner

Thanks, looks good!
Only thing I'm thinking is that LOGGIN_LEVEL* and HPI_LOGS have inconsistent naming. Perhaps we could use LOGGING_LEVEL_HPI as the primary control instead, and use HPI_LOGS as a legacy fallback? Not a huge deal though.

@purarue
Copy link
Contributor Author

purarue commented Sep 7, 2023

Yeah, I think I mentioned the same in #305, can update the docs to something more consistent

@purarue purarue force-pushed the logging-doc-updates branch from 21fd6bb to 31c7013 Compare September 7, 2023 01:14
re-adds a removed check for HPI_LOGS, add some docs

fix the checks for browserexport/takeout logs to
use the computed level from my.core.logging
@purarue purarue force-pushed the logging-doc-updates branch from 31c7013 to 374ecaf Compare September 7, 2023 01:16
@purarue
Copy link
Contributor Author

purarue commented Sep 7, 2023

Should be good, added a my.core.warnings.medium incase the user had HPI_LOGS set

@karlicoss karlicoss merged commit 2a46341 into karlicoss:master Sep 7, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants