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

logging best practice #724

Merged
merged 9 commits into from
Jun 6, 2023
Merged

logging best practice #724

merged 9 commits into from
Jun 6, 2023

Conversation

emanuel-schmid
Copy link
Collaborator

@emanuel-schmid emanuel-schmid commented May 16, 2023

Changes proposed in this PR:

  • add a novel config value logging.climada_style that controls whether or not the logging is configured in the traditional climada style.

This PR fixes #265

PR Author Checklist

PR Reviewer Checklist

@emanuel-schmid emanuel-schmid marked this pull request as draft May 16, 2023 13:52
@emanuel-schmid emanuel-schmid changed the title Feature/unusurpate logging logging best practice May 16, 2023
@tovogt
Copy link
Collaborator

tovogt commented May 24, 2023

Thanks @emanuel-schmid, this implementation looks very clean to me!

I would be happy if somebody came up with a more fitting name for the config parameter instead of logging.climada_style but I have no strong feelings. It just seems to me like it's not about the "style" of the logging, but about whether climada is in control of the logging, or whether control of the logging is left to the user. Maybe logging.managed?

@emanuel-schmid
Copy link
Collaborator Author

Thanks @tovogt . Changed it to managed - not to close the discussion, just because it's already much better.

@emanuel-schmid emanuel-schmid requested a review from petermkr June 5, 2023 09:16
@emanuel-schmid emanuel-schmid marked this pull request as ready for review June 5, 2023 09:17
@tovogt
Copy link
Collaborator

tovogt commented Jun 6, 2023

I'm happy with this PR conceptually. Please just don't forget to update the name of the config parameter ("managed" or "climada_style" or whatever) in the CHANGELOG and in the docs, once the decision for the name is final.

CHANGELOG.md Outdated Show resolved Hide resolved
@emanuel-schmid emanuel-schmid merged commit a60241a into develop Jun 6, 2023
@emanuel-schmid emanuel-schmid deleted the feature/unusurpate_logging branch June 6, 2023 16: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.

2 participants