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

Update logger #1044

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Conversation

jverswijver
Copy link
Contributor

small changes to logging behavior:

  • add check to update the logger when the dj.config is updated
  • make sure that environment variables are given priority over the config
  • should work with context manager now

@jverswijver jverswijver marked this pull request as draft August 17, 2022 16:45
@guzman-raphael
Copy link
Collaborator

@jverswijver From our prior discussion, the largest issue I see here is that if we are properly addition loglevel to be used in the config, we should verify the case where a user has a dj_local_conf.json and first imports datajoint. Their log level should be appropriately set.

@jverswijver
Copy link
Contributor Author

Yes, we should try to look for the config and if it is there we should load the log level for the user when we create the logger. Except of they have the log level set in their env which should take priority.

@@ -248,6 +248,9 @@ def __setitem__(self, key, value):
self._conf[key] = value
else:
raise DataJointError("Validator for {0:s} did not pass".format(key))
valid_logging_levels = ["DEBUG", "INFO", "WARNING", "ERROR", "CRITICAL"]
if "loglevel" in key and value in valid_logging_levels:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if "loglevel" in key and value in valid_logging_levels:
if "loglevel" == key and value in valid_logging_levels:

Copy link
Member

Choose a reason for hiding this comment

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

should probably raise an error if value is not valid?

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.

None yet

3 participants