Repository navigation
fix: leave logging configuration to the importing application - #76
Conversation
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf688ba8e3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b3b6500df
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80f861dd99
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
jm-rivera
left a comment
jm-rivera
left a comment
There was a problem hiding this comment.
Thanks @tillywoodfield just a comment on version
…1.13.0 install_legacy_logger_name() now emits the DeprecationWarning when the oda_importer logger already has a level, handler or filter, so a configuration made before importing oda_reader warns as well. The release becomes 1.13.0, since it deprecates a public logger name.
jm-rivera
left a comment
jm-rivera
left a comment
There was a problem hiding this comment.
Approved. I pushed de10272: the release is now 1.13.0, since it deprecates a public logger name, and install_legacy_logger_name() warns when oda_importer already has a level, handler or filter at import, so the changelog holds for configuration made before the import too.
Closes #68
Importing oda_reader called
logging.basicConfig(level=logging.INFO, ...)inoda_reader/common.py, which gave the host application a root handler, an INFO level and oda_reader's format, and turned the application's own laterbasicConfiginto a no-op. That call is gone. Every module logs throughlogging.getLogger(__name__), so all of the package's messages sit underoda_readerwith no handler or level set, and an unconfigured application sees only warnings and errors.The
oda_importername keeps working until 2.0 throughoda_reader/_logging.py. On import, the package takes theoda_importerlogger, swaps its class to one whosesetLevel,addHandlerandaddFilterraise aDeprecationWarningonce per process, and makes it the parent of theoda_readerlogger. Levels, handlers andpropagate = Falseset onoda_importerthen reach every package record through the normal parent walk, and this works whether the application configuresoda_importerbefore or after the import. A level, handler or filter already onoda_importerwhen the package is imported triggers the same warning. The publicoda_reader.common.loggeris that sameoda_importerlogger, so code that configured it keeps the same reach, and the install step clears the logging manager's level caches so a level evaluated before the import doesn't go stale. Registering the package logger object under the old name, as dcp-tools did, gives no warning, and adictConfigthat names onlyoda_reader(with the defaultdisable_existing_loggers) then disables the package logger through the alias key. The test for that case fails under the alias approach.Three uses of
oda_importerstop affecting the package, and the changelog says so. AdictConfigorfileConfigthat namesoda_importerwith the defaultdisable_existing_loggersdisables theoda_reader.*module loggers, because they are not its children by name. Setting.disabledon it, or adding a logger-level filter, has no effect, because logger filters run only on the logger that creates the record. The class swap is the part to check most closely.docs/docs/advanced.mdgains an "Enable logging" section on turning the package's logging up or down, replacing the claim that it had no logging. The release is 1.13.0, since it deprecates a public logger name, with the changelog showing how to turn INFO messages back on.Testing: ruff check, ruff format --check, ty check and the unit suite (
pytest -n auto -m "not integration", 687 passed, 8 skipped) ran locally on Python 3.13, and CI passed on all nine OS and Python combinations. The newtests/common/unit/test_logging.pyruns import-state,dictConfigand configured-before-import cases in fresh interpreters. Restoring thebasicConfigline fails the import test, and swapping in the dcp-tools alias fails thedictConfigtest. Over repeated runs,test_register_bulk_file.py::TestOfflineRead::test_online_hit_records_dataset_for_offline_readsfailed once in about fifteen, finding more than one bulk-cache record. That file and the cache logic it exercises are untouched here.🤖 Generated with Claude Code