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

Refactor logging configuration #933

Merged
merged 7 commits into from
Jan 12, 2021
Merged

Refactor logging configuration #933

merged 7 commits into from
Jan 12, 2021

Conversation

stefsmeets
Copy link
Contributor

@stefsmeets stefsmeets commented Jan 8, 2021

Description

This code moves the logging out of the _config.py module into its own _logging.py, so that the logger can be imported without initializing DIAGNOSTICS_PATH / CFG / etc. I also did some refactoring and added some documentation based on my understanding of this module. No functionality has been added or changed.

I hope that this will make it easer to work with. This is also needed for #907 for I'm looking to see if I can merge logging from there with the logging config that already exists.


Before you get started

Checklist


To help with the number pull requests:

@valeriupredoi
Copy link
Contributor

valeriupredoi commented Jan 8, 2021

nice one @stefsmeets - I think this is a cleaner more portable approach! One remark - would you be able to write a couple tests for the new module, it's good to have it test-covered as a module even if (I think) there were minimal tests before 👍 🍺

@stefsmeets
Copy link
Contributor Author

Yeah, I could not find any tests so I did not bother but I can add a couple next week. 👍

@valeriupredoi
Copy link
Contributor

cheers dude! Lemme know if you got time, if not I can do that, been doing a lot of maintenance and test stuff lately anyway 👍

@stefsmeets
Copy link
Contributor Author

Hi @valeriupredoi , I added a couple of tests to cover the functionality of module. Does this work for you?

@valeriupredoi
Copy link
Contributor

very cool @stefsmeets - 100% coverage too 🍺

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants