Improved logging#252
Conversation
rozyczko
added
[scope] enhancement
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #252 +/- ##
===========================================
+ Coverage 82.55% 82.92% +0.37%
===========================================
Files 62 62
Lines 4946 5084 +138
===========================================
+ Hits 4083 4216 +133
- Misses 863 868 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
|
damskii9992
left a comment
damskii9992
left a comment
There was a problem hiding this comment.
I think we should have an ADR on the logger to explain why we implemented it, and instructions on how to use it, both when writing to it, and when ignoring it (I guess the last one can simply refer to the .md file you wrote).
| @pytest.fixture(autouse=True) | ||
| def _quiet_easyscience(): | ||
| with global_object.log.at_level(logging.ERROR): | ||
| yield |
There was a problem hiding this comment.
How does this work? You're only muting the log in the context, so how would this actually mute the log in a test? Wouldn't
global_object.log.set_level('ERROR')Be better?
There was a problem hiding this comment.
autouse fixture yields inside with at_level(logging.ERROR), so the test body runs within the context and is muted. Your set_level('ERROR') also works and is marginally simpler, but the code is correct
| - **No default stream handlers** — EasyScience does not attach handlers | ||
| that write to `stdout` or `stderr`. It only creates log records. | ||
| Applications and test frameworks decide where those records go. |
There was a problem hiding this comment.
Isn't this bad? What about users that use easyscience as a standalone fitting package? Do they not see any messages without configuring it? They should . . .
There was a problem hiding this comment.
Python's logging.lastResort handler emits WARNINGs and above to stderr when no handlers are
configured, so standalone easyscience users do see warnings/errors out of the box.
Only INFO/DEBUG are hidden without change of setup.
| upper = stripped.upper() | ||
| if upper in _LEVEL_NAME_MAP: | ||
| return _LEVEL_NAME_MAP[upper] | ||
| return default |
There was a problem hiding this comment.
You should probably raise in the end. If you try to set a logging level, but make a spelling mistake, we shouldn't silently return the default.
Also, why the hell is the default a separate parameter? Why not just the default of raw? Which probably also should have a better parameter name (level?)
There was a problem hiding this comment.
Actually, this method probably shouldn't have any defaults . . .
There was a problem hiding this comment.
silently falling back to a default on a bad value is the right behaviour. You don't want import to crash on a typo'd env var. This is for env var parsing.
FOr public API (set_level/at_level) tthis should raise. I think we need to split the concerns here
Strict parse for the API
lenient parse (default) for env var
| logging.getLogger('easyscience.deprecated').warning( | ||
| 'Call to deprecated function %s.', | ||
| func.__name__, | ||
| stacklevel=3, |
There was a problem hiding this comment.
stacklevel? Is that a feature in the logging module? You haven't used this elsewhere.
There was a problem hiding this comment.
stacklevel is a supported kwarg on logging methods and here it correctly points the warning at the caller of the deprecated function.
Not having it elsewhere is a fair point. I'll add it to other deprecation log locations for consistency
| 'COM812', # https://docs.astral.sh/ruff/rules/missing-trailing-comma/ | ||
| # The following is replaced by 'D'/[tool.ruff.lint.pydocstyle] and [tool.pydoclint] | ||
| 'DOC', # https://docs.astral.sh/ruff/rules/#pydoclint-doc | ||
| # The following is replaced by 'D'/[tool.ruff.lint.pydocstyle] and [tool.pydoclint] 'DOC', # https://docs.astral.sh/ruff/rules/#pydoclint-doc |
There was a problem hiding this comment.
You also accidentally reverted this to comment out the DOC rule :)
There was a problem hiding this comment.
Cause pixi run check is messing it up!!!
I need to change it once and for all
|
damskii9992
added
[area] global_object
damskii9992
left a comment
damskii9992
left a comment
There was a problem hiding this comment.
LGTM now with a single addition ;)
| """Convert a level number or name into a numeric level, raising on an unknown name.""" | ||
| if isinstance(level, int): | ||
| return level | ||
| stripped = level.strip() |
There was a problem hiding this comment.
What if level was something else than an int or str?
Since you're using the method in the level setter, you need to check the input type.
There was a problem hiding this comment.
Good catch. this is called directly from the setter, so isn't guaranteed the right type.
|
And don't forget to finalize the ADR suggestion into a real ADR: |
2 participants
Switched from direct
warnings.warnandprintstatements to Python's standardloggingmodule throughout the codebase.Replaced all uses of
warnings.warnandprintfor runtime and deprecation warnings with appropriate calls to theloggingmodule, using named loggers under theeasysciencehierarchy for better control and filtering.Added a new user guide page,
controlling-log-output.md, detailing how to suppress, filter, or redirect EasyScience log messages.No global reconfiguration, no default stream handlers, and all loggers inherit configuration, giving users and downstream libraries full control over log output.