-
Notifications
You must be signed in to change notification settings - Fork 16
Refactor xCDAT logging for consistency and safety #811
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the logging configuration in xcdat by moving it from module-level initialization to a function-based approach. The changes improve code organization and provide better control over when logging is configured.
- Converted logging constants to uppercase naming convention (
log_format→LOG_FORMAT, addedLOG_LEVEL) - Encapsulated logging setup logic in
_setup_root_logger()function - Added warning capture functionality and improved documentation
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| xcdat/_logger.py | Refactored logging setup from module-level to function-based approach with _setup_root_logger(), updated constant naming to follow conventions, and added captureWarnings support |
| xcdat/init.py | Added import and call to _setup_root_logger() to initialize logging when the package is imported |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #811 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 16 16
Lines 1784 1794 +10
=========================================
+ Hits 1784 1794 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Description
Summary
This PR refactors xCDAT’s logging system to align with Python library best practices and ensure consistent, predictable log behavior across all modules.
It introduces a dedicated
xcdatpackage logger, standardizes log formatting, prevents duplicate console output, removes intrusive global logging configuration, and ensures handler levels remain synchronized with the logger’s level.Changes
Introduced
_setup_xcdat_logger()xcdatpackage logger (not the root logger).forceand conditional setup logic.Updated
__init__.py_setup_xcdat_logger()once on import to initialize a consistent logging configuration for all submodules.Removed old root logger setup
logging.basicConfig()and import-time handler creation.Dropped automatic warning capture
logging.captureWarnings(True)to avoid modifying global warning handling.Behavior Improvements
xcdatloggerRationale
This refactor makes xCDAT’s logging:
Checklist
If applicable: