change logger from root logger to dedicated name#63
Conversation
There was a problem hiding this comment.
Hello @skjerns, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello! Gemini here, providing a summary of this pull request by @skjerns.
The primary goal of this PR, as indicated by the title and description, is to improve the logging within the mat73 library by switching from using the default root logger directly to using a dedicated logger instance named 'mat73'. This is a standard and recommended practice in Python logging, as it allows users of the library to configure logging for mat73 specifically, without affecting or being affected by logging configurations elsewhere in their application.
The changes are contained within a single file, mat73/core.py, and involve initializing a named logger and then replacing all existing calls to logging.warning and logging.error with calls to this new logger instance.
Highlights
- Logging Refactor: The pull request refactors the logging implementation to use a dedicated logger instance (
logging.getLogger('mat73')) instead of the root logger (logging). - Improved Configurability: Using a named logger allows users of the library to configure logging specifically for
mat73messages, providing better control and isolation.
Changelog
- mat73/core.py
- Added initialization of a dedicated logger instance:
logger = logging.getLogger('mat73'). - Replaced all direct calls to
logging.warningwithlogger.warning. - Replaced all direct calls to
logging.errorwithlogger.error.
- Added initialization of a dedicated logger instance:
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Root logger's reach,
A named logger finds its place,
Logs flow, clear and clean.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request effectively changes the logging mechanism from using the root logger to a dedicated named logger, mat73. This is a commendable improvement as it allows for more granular control over logging levels and outputs for this specific library, enhancing maintainability and debuggability. The changes are mostly direct replacements of logging.<level>() calls with logger.<level>().
I've found one area for improvement related to how tracebacks are logged, detailed in the comments. Other minor changes involve whitespace adjustments, which are generally positive cleanups.
Summary of Findings
- Adoption of Named Logger: The core change to use
logging.getLogger('mat73')instead of the root logger is a significant improvement. It aligns with best practices for library logging, allowing consumers of the library to configuremat73logs independently. - Exception Traceback Logging: In one of the error handling blocks, the exception traceback is logged by manually appending
e.__traceback__to the log message. The recommended approach is to uselogger.error(..., exc_info=True)orlogger.exception(), which ensures the full traceback is properly captured and formatted by the logging module. A specific comment has been made for this. - Minor Whitespace and Formatting Cleanups: The pull request includes several minor whitespace changes, such as removing trailing whitespace and replacing lines with only spaces with truly empty lines (e.g., mat73/core.py lines 129, 311, 317 on the right side). These are good stylistic cleanups. (Severity: low - not commented inline due to review settings).
Merge Readiness
The pull request is a positive step towards better logging practices. There is one medium severity suggestion regarding the method of logging exception tracebacks that should be addressed to ensure robustness and adherence to standard Python logging conventions. Once this is addressed, the PR should be in good shape for merging. As a reviewer, I am not authorized to approve the pull request; please ensure further review and approval as per your team's process.
mat73/core.py
Outdated
| logger.error(f'Tried loading the sparse matrix `{hdf5.name}`' | ||
| ' but something went wrong: {e}\n{e.__traceback__}') |
There was a problem hiding this comment.
For logging exceptions along with their tracebacks, it's generally more robust and idiomatic in Python to use logger.exception() (if the log level is ERROR) or pass exc_info=True to logging methods like logger.error(). This allows the logging framework to handle the formatting of the traceback information correctly and consistently.
Manually including e.__traceback__ in an f-string might not always provide the most readable or complete traceback, and exc_info=True (or logger.exception()) is the standard way to achieve this.
Could this be updated to use exc_info=True for more standard traceback logging?
logger.error(f'Tried loading the sparse matrix `{hdf5.name}` but something went wrong: {e}', exc_info=True)
see title