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

Fix logging pattern #103

Merged
merged 9 commits into from
Feb 24, 2025
Merged

Fix logging pattern #103

merged 9 commits into from
Feb 24, 2025

Conversation

AndyRae
Copy link
Member

@AndyRae AndyRae commented Feb 24, 2025

Pull Request Contents

♻️ Refactor

Description

This PR fixes the logging pattern to a more pythonic approach - due to how the codebase had evolved, it had a mixed methods approach: some dependency injection (explicitly passing the logger to each function), and some module level singleton (importing it).

This wasn't good - and essentially introduced uncertainty into new logging (which pattern to use where), and how settings were then being instantiated in the two entrypoints. This shown in #92 where we had to rerun the settings (by importing SyncDBManager) to avoid them being cleared where logging was instantiated later in the app.
(with this change, we are now able to remove the import of SyncDBManager that fixed that issue, and make settings instantiation cleaner)

This PR therefore configures the logging once at each entrypoint, and enables each module to import logger instead of it being repeatedly injected and further boilerplate code. This is generally accepted as the Python approach to handling logging configuration: https://docs.python.org/3/howto/logging-cookbook.html#using-logging-in-multiple-modules

Screenshots, example outputs/behaviour etc.

✅ Added/updated tests?

  • This PR contains relevant tests
    • Or doesn't need to per the below explanation
  • I've completed all actions and tasks detailed in the PR Template

Copy link

Coverage

Coverage Report
FileStmtsMissCoverMissing
src/hutch_bunny
   cli.py34340%1–60
   daemon.py18180%1–32
src/hutch_bunny/core
   db_manager.py723847%35, 49, 60, 71, 87, 89, 91, 93, 95, 109, 121–126, 129–132, 163–182, 194–199, 208–211, 219
   enums.py8188%23
   execute_query.py272122%28–62
   obfuscation.py17194%65
   parser.py550%1–22
   results_modifiers.py13469%40–45
   setting_database.py271737%17, 20, 28–63
   settings.py45296%64, 89
src/hutch_bunny/core/rquest_dto
   activity_job.py12120%1–36
   base_dto.py6267%3, 7
   cohort.py14471%17, 32–34
   file.py12192%24
   group.py14471%19, 34–36
   query.py381463%32, 51–52, 79, 102–116
   result.py15193%38
   rule.py503236%28–31, 41–52, 64–72, 85–101
src/hutch_bunny/core/solvers
   availability_solver.py1807757%149–150, 155, 182, 198–224, 232, 256–276, 292–301, 325–331, 338–410, 420–429, 436–476, 495–511, 525–531
   query_solvers.py150696%36, 142, 253, 408–410
src/hutch_bunny/core/upstream
   task_api_client.py482744%25–27, 44–54, 70–71, 89–90, 100–119
   task_handler.py990%1–37
TOTAL100933067% 

Tests Skipped Failures Errors Time
33 0 💤 0 ❌ 0 🔥 1.416s ⏱️

@AndyRae AndyRae marked this pull request as ready for review February 24, 2025 17:32
@AndyRae AndyRae requested a review from prquinlan February 24, 2025 18:37
@AndyRae AndyRae merged commit b8b6715 into main Feb 24, 2025
5 checks passed
@AndyRae AndyRae deleted the fix/settings-injection branch February 24, 2025 19:01
@AndyRae AndyRae mentioned this pull request Feb 25, 2025
3 tasks
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