Skip to content

Conversation

@MatthieuCMira
Copy link
Contributor

@MatthieuCMira MatthieuCMira commented Aug 28, 2025

GEOPY-2420 - change get_logger function to define setLevel
update get_logger to accept set_level as a string.

Improve set logger tests.

@github-actions github-actions bot changed the title GEOPY-2420 GEOPY-2420: change get_logger function to define setLevel Aug 28, 2025
@codecov
Copy link

codecov bot commented Aug 28, 2025

Codecov Report

❌ Patch coverage is 67.74194% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.77%. Comparing base (111b167) to head (0c93d04).

Files with missing lines Patch % Lines
geoapps_utils/utils/logger.py 67.74% 9 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #151      +/-   ##
===========================================
- Coverage    84.91%   84.77%   -0.15%     
===========================================
  Files           19       19              
  Lines          988     1018      +30     
  Branches       130      139       +9     
===========================================
+ Hits           839      863      +24     
- Misses         112      119       +7     
+ Partials        37       36       -1     
Files with missing lines Coverage Δ
geoapps_utils/utils/logger.py 79.59% <67.74%> (+0.64%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

level_name: bool = True,
propagate: bool = True,
add_name: bool = True,
level: str | LoggerLevel | None = None,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even wonder if we should not simplify and just expect a Literal[logging.WARNING, logging.INFO...]. It's not that much work on the other side to just import logging if you want to make specific cases.

Copy link
Contributor Author

@MatthieuCMira MatthieuCMira Aug 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it's something we are using everywhere, we can have the luxury to create our loggers from a string in this function. That's for us.
It's not a lot of work, but quite convenient to just create a logger this way!

benk-mira
benk-mira previously approved these changes Aug 29, 2025
Copy link
Contributor

@benk-mira benk-mira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should approve this as long as it meets our immediate needs. Ie: solves Matthieu's need for multiple levels of logging, fixes double printing in scigeoh5/plate-simulation, and doesn't interfere with existing logging behaviour. The we should open up an issue to explore Seb's idea, and propagate a unified logging system to all applications through the base driver.

Copy link
Contributor

@sebhmg sebhmg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating a separate logger for each level is not the recommended approach.
Please postpone these changes until we can devise a more robust solution, ideally leveraging Python’s built-in logging features.

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.

5 participants