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

Use logging.info for ingest/outgest prints #186

Merged
merged 5 commits into from
Jun 24, 2022
Merged

Use logging.info for ingest/outgest prints #186

merged 5 commits into from
Jun 24, 2022

Conversation

johnkerl
Copy link
Member

@johnkerl johnkerl commented Jun 23, 2022

Also manually verified ingestor -q and outgestor -q are silent.

@johnkerl johnkerl requested review from Shelnutt2 and gsakkis June 23, 2022 16:12
@johnkerl johnkerl force-pushed the kerl/logger branch 3 times, most recently from 47f6b42 to 46da39b Compare June 23, 2022 21:51
Copy link
Contributor

@gsakkis gsakkis left a comment

Choose a reason for hiding this comment

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

This is perfectly fine for scripts or more generally applications that have full control over logging. However it is not ideal for libraries that need to play well with others: the logging system if quite flexible so that the driver application can enable logging for library X1 at level L1 with handler(s) H1 and formatter F1, for library X2 at level L2 with handler(s) H2 and formatter F2, and so on.

To make it a well-behaved library, two things need to change:

  1. Replace the logging.info calls with logger.info, where logger is a logging.Logger instance. A common practice is to create one logger for each module (that needs to log anything):
import logging

logger = logging.getLogger(__name__)
  1. Remove the logging.basicConfig (or any kind of logging configuration for that matter) from the library. Configuration is the responsibility of the driver application, not an individual library. In this case this implies that the verbose flag from SOMA and SOMACollection should go.
    Having said that, the library may configure logging in its entry points (e.g. CLI scripts), in this case for tools and examples.

More about Python logging:

@johnkerl johnkerl force-pushed the kerl/logger branch 2 times, most recently from 434c7e4 to 3b47714 Compare June 24, 2022 14:50
@johnkerl johnkerl force-pushed the kerl/logger branch 5 times, most recently from 80b500b to b122ebe Compare June 24, 2022 20:07
@johnkerl johnkerl marked this pull request as draft June 24, 2022 20:52
@johnkerl johnkerl force-pushed the kerl/logger branch 5 times, most recently from 31866fe to 7343f01 Compare June 24, 2022 21:10
@johnkerl johnkerl marked this pull request as ready for review June 24, 2022 21:22
@johnkerl johnkerl merged commit 537b22d into main Jun 24, 2022
@johnkerl johnkerl deleted the kerl/logger branch June 24, 2022 21:26
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.

3 participants