Skip to content
This repository was archived by the owner on Jan 13, 2025. It is now read-only.

Conversation

@andres-fr
Copy link

This is a proposed PR to fix issue #26 (disabling logging).

Since a new logger is being created for several files when the lib is loaded, it is already late/cumbersome for the user to keep track of the internals and modify the logging behaviour. This proposed fix addresses this issue as follows:

  1. Loggers are singletons inside the log_util namespace. This way we can keep track of all loggers in a centralized way. This is implemented by
    a) using a dictionary to keep all logs
    b) making the log creator a protected function and
    c) the log getter tries to fetch an existing singleton, and otherwise creates+returns one if it didn't exist

  2. The module incorporates a log_to_directory function that provides the desired functionality to the specified loggers (or to all if none specified). I.e. default behaviour is not logging, and users can opt-in at any time and also provide their desired destiny. This function has been added to __init__ in order to keep it more accessible to users.

The user simply has to add the following lines anywhere inside their application code:

import elsapy
elsapy.log_to_directory("logs")

I think this is an improvement for the following reasons:

  1. Most users won't like their filesystem to be populated without explicitly requesting so
  2. If users want this functionality, this way they can also select the target directory
  3. It is very easy for the users
  4. It is fairly easy for the maintainers, and extendible: Further functionality (like e.g. removing file handlers) can be added in an analogous manner

Note that the use of a singleton is the least intrusive fix given that several modules are creating loggers as side effect when loaded.

Let me know what you think! Hope this helps.

Cheers,
Andres

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant