Skip to content

feat: unbundle delphi-logger and re-export it in delphi-utils #1970

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

Closed
wants to merge 2 commits into from

Conversation

dshemetov
Copy link
Contributor

Description

Refactor delphi-utils to depend on delphi-logger standalone repo.

Changelog

  • Moved logger.py to the standalone delphi-logger repo.
  • Now it's importable, so import it in delphi-utils and re-export it, so that from delphi-utils import get_structured_logger still works.

Fixes

@dshemetov dshemetov requested review from melange396 and nmdefries June 5, 2024 21:11
@dshemetov dshemetov force-pushed the ds/logger branch 2 times, most recently from ff93c62 to 1a9ceb0 Compare June 5, 2024 21:38
@dshemetov
Copy link
Contributor Author

See this snag here.

Copy link
Contributor

@nmdefries nmdefries left a comment

Choose a reason for hiding this comment

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

Couple things to verify, but otherwise looks good.

pool.join()
finally:
logger_thread.stop()
from delphi_logger import get_structured_logger # pylint: disable=unused-import
Copy link
Contributor

Choose a reason for hiding this comment

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

question: this bit does the reexporting?

get_structured_logger
from typing import Any, Dict

from delphi_utils import add_prefix, create_export_csv, get_structured_logger, pool_and_threadedlogger
Copy link
Contributor

Choose a reason for hiding this comment

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

question: but in delphi_utils, we're only reexporting get_structured_logger, so won't importing pool_and_threadedlogger here fail?

@@ -1,255 +1,10 @@
"""Structured logger utility for creating JSON logs.
"""Temporary migration compatibility file.
Copy link
Contributor

Choose a reason for hiding this comment

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

note: we'll need a followup issue once we remove this file to make sure that other logger imports continue to work, e.g..

@dshemetov
Copy link
Contributor Author

Closed for another approach (see comment here).

@dshemetov dshemetov closed this Jul 1, 2024
@dshemetov dshemetov deleted the ds/logger branch July 1, 2024 18:53
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