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

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
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